Deduplicate module names in FCS #2772
Conversation
|
Jus for sake of complteness. It seems fsi.exe is fine:
|
| @@ -0,0 +1,13 @@ | |||
| <Project Sdk="FSharp.NET.Sdk;Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
Does this test anything valuable? After all it is not using the locally built CompilerService.
There was a problem hiding this comment.
I'm not sure about tempet.fsproj, but the tests in tests/service/ProjectAnalysisTests.fs are, I think, also being run against FSharp.LanguageService.Compiler.dll (the locally built compiler service DLL)
There was a problem hiding this comment.
yes it's a NUnit test that loads the proj. I think it doesn't use the sdk at all
|
Yeah I guess the test is not correct. But the fix is. Will investigate on
Monday
|
|
@KevinRansom The test is necessary for FCS as unfortunately the fix from #2728 didn't apply when calling Please note the logic in this PR it's the same as in #2728 applied to a different place. Maybe we could leave a comment as a reminder to refactor both fixes to a common place in the future? |
|
@alfonsogarciacaro @KevinRansom I think the fsproj file is not used at all. the test itself is in NUnit |
|
ok further tracing showed that the fix actually deduped the module but it still fails in tast.fs - CombineModuleOrNamespaceTypes so I need to do more investigation .... |
|
parse D:\j\workspace\release_ci_pa---3f142ccc\tests\service/data/samename/folder1/a.fs ERROR: Two modules named 'tempet.SayA' occur in two parts of this assembly So it's complaing about the module SayA lol - I didn't copy the test correctly.
|
|
Ok fixed. Ready to go in |
src/fsharp/vs/IncrementalBuild.fs
Outdated
| @@ -1328,6 +1328,14 @@ type IncrementalBuilder(tcGlobals,frameworkTcImports, nonFrameworkAssemblyInputs | |||
| let StampFileNameTask (cache: TimeStampCache) _ctok (_m:range, filename:string, _isLastCompiland) = | |||
| assertNotDisposed() | |||
| cache.GetFileTimeStamp filename | |||
|
|
|||
| // Deduplicate module names | |||
| let seen = Dictionary<string,Set<string>>() | |||
There was a problem hiding this comment.
@forki This duplicates the code added to fsc.fs? Could you extract this to CompileOptions.fs or CompileOps.fs and share it please? https://github.com/Microsoft/visualfsharp/blob/f6e160a2e4a375754bcf62fc3641b1861093ca3b/src/fsharp/fsc.fs#L1768
|
I also send a Pull request to @alfonsogarciacaro's PR so that both are in sync |
|
can you please merge and release the corresponding FCS PR? It's needed for ionide ;-) |
|
yes will do |
|
done, 12.0.3 |
|
I came across same/similar issue. Here is the smallest repro I could come up with https://github.com/ThisFunctionalTom/FsxLoadBugRepro Maybe it can help. |
this ports the work of @alfonsogarciacaro from FCS back to VF#
/see also fsharp/fsharp-compiler-docs#749