-
Notifications
You must be signed in to change notification settings - Fork 790
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
Fix de-duplication of module names #6086
Conversation
@forki You might like to take a look at the move to use immutable state for the-duplication information |
So you are using a map and passing it down? Sounds good |
Yes, and the disambiguation effectively moves from parsing to type checking (since the incremental builder is set up to allow parsing files in parallel, independently. If necessary we could easily do the disambiguation earlier if when processing the file names it's a very cheap step.) |
@cartermp @KevinRansom I think this can now be merged, it unblocks using VisualFSharp.sln for error-free editing |
if count = 1 then qualifiedNameOfFile else QualifiedNameOfFile(Ident(id.idText + "___" + count.ToString(), id.idRange)) | ||
let DeduplicateModuleName (moduleNamesDict:ModuleNamesDict) fileName (qualNameOfFile: QualifiedNameOfFile) = | ||
let path = Path.GetDirectoryName fileName | ||
let path = if FileSystem.IsPathRootedShim path then try FileSystem.GetFullPathShim path with _ -> path else path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often is DedeplicateModuleName
called? I worry that this line could have a similar effect as #5932
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once per check of a file - i.e. very rarely (In #5932 the GetFullPath was called like every identifier and every token in a file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
The de-duplication of module names added somewhere in VS2017, see #5979.
used mutable state which mucked with re-computation in IncremenetalBuild.fs
didn't normalize paths such as
c:\foo\bar\..\abc.fsi
, which were being provided for signature files in complex solutions like VisualFSharp.sln after the switch over to the new SDK-style project files for some reason.Testing is so far manual on VisualFSharp.sln