Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Legacy module names option #5679

Merged
merged 7 commits into from
Aug 12, 2024

Conversation

robin-aws
Copy link
Member

Description

Similar to the previously added --legacy-data-constructors option, --legacy-module-names outputs module names in the scheme A_mB_mC, as previous versions of Dafny did, instead of A.B.C, in the name of backwards compatibility with other Dafny-generated consumers.

How has this been tested?

Updated comp/separate-compilation/usesTimesTwo-oldDafnyCompatibility.dfy to use a nested library module name, and therefore test both --legacy-data-constructors and --legacy-module-names.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

// Use an "underscore-escaped" character as a module name separator, since
// underscores are already used as escape characters in SanitizeName()
compileName = EnclosingModule.GetCompileName(options) + options.Backend.ModuleSeparator + NonglobalVariable.SanitizeName(Name);
if (options.Get(LegacyModuleNames)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make it so that read translation record options are automatically stored in the module specific options that are attached to ModuleDecl, and then let the options here be those module specific options, so that if a Dafny 4.7 project that does not use --legacy-module-names, imports a Dafny 4.7 project that does use --legacy-module-names, it still works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that in mind as a future improvement when I added translation records too.

@keyboardDrummer
Copy link
Member

keyboardDrummer commented Aug 12, 2024

The build failure is because the property --outer-module is not tracked for the prefix module CoolLibraryName, because of this code

// This is primarily here to exclude prefix modules
// (e.g. something like A.B that only appears in a module A.B.C { ... } declaration)
// since those can appear in multiple separately-compiled projects. 
if (ModuleEmptyForCompilation(module)) {
  continue;
}

I'm having a bit of a blackout with regards to coming up with a good solution :D
A quickfix might be to specifically store the option--outer-module, even for empty modules, and then potentially to check that this value matches when it's specified multiple times for the same module.

Can you refresh my memory on the use-case that was problematic here? Why do different projects need to have outermost modules with the same name? We also had issues with collisions of declarations in the default module right?

Should projects have a name, which serves like an --outer-module but at the Dafny level, where combining projects triggers a check that the names of all combined projects are unique?

Copy link
Member

@keyboardDrummer keyboardDrummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but it does not build.

@robin-aws
Copy link
Member Author

Fixed the build by avoiding the empty parent module in the test, since it's not directly relevant to the new option. But it's a valid concern so I cut #5683 for follow up.

@robin-aws robin-aws enabled auto-merge (squash) August 12, 2024 18:02
@robin-aws robin-aws merged commit d38be44 into dafny-lang:master Aug 12, 2024
21 checks passed
@robin-aws robin-aws deleted the legacy-module-names-option branch August 12, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants