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

Deduplicate module names in FCS #2772

Merged
merged 5 commits into from
Apr 3, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion src/fsharp/vs/IncrementalBuild.fs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 2fc7247

let deduplicate (paths: Set<string>) path (qualifiedNameOfFile: QualifiedNameOfFile) =
let count = if paths.Contains path then paths.Count else paths.Count + 1
seen.[qualifiedNameOfFile.Text] <- Set.add path paths
let id = qualifiedNameOfFile.Id
if count = 1 then qualifiedNameOfFile else QualifiedNameOfFile(Ident(id.idText + "___" + count.ToString(),id.idRange))

/// This is a build task function that gets placed into the build rules as the computation for a VectorMap
///
Expand All @@ -1344,8 +1352,31 @@ type IncrementalBuilder(tcGlobals,frameworkTcImports, nonFrameworkAssemblyInputs

try
IncrementalBuilderEventTesting.MRU.Add(IncrementalBuilderEventTesting.IBEParsed filename)
let result = ParseOneInputFile(tcConfig,lexResourceManager, [], filename ,isLastCompiland,errorLogger,(*retryLocked*)true)
let input = ParseOneInputFile(tcConfig,lexResourceManager, [], filename ,isLastCompiland,errorLogger,(*retryLocked*)true)
fileParsed.Trigger (filename)
let result =
match input with
| Some(ParsedInput.ImplFile (ParsedImplFileInput.ParsedImplFileInput(fileName,isScript,qualifiedNameOfFile,scopedPragmas,hashDirectives,modules,(isLastCompiland,isExe)))) ->
let path = Path.GetDirectoryName fileName
match seen.TryGetValue qualifiedNameOfFile.Text with
| true, paths ->
let qualifiedNameOfFile = deduplicate paths path qualifiedNameOfFile
let input = ParsedInput.ImplFile(ParsedImplFileInput.ParsedImplFileInput(fileName,isScript,qualifiedNameOfFile,scopedPragmas,hashDirectives,modules,(isLastCompiland,isExe)))
Some input
| _ ->
seen.Add(qualifiedNameOfFile.Text,Set.singleton path)
input
| Some(ParsedInput.SigFile (ParsedSigFileInput.ParsedSigFileInput(fileName,qualifiedNameOfFile,scopedPragmas,hashDirectives,modules))) ->
let path = Path.GetDirectoryName fileName
match seen.TryGetValue qualifiedNameOfFile.Text with
| true, paths ->
let qualifiedNameOfFile = deduplicate paths path qualifiedNameOfFile
let input = ParsedInput.SigFile (ParsedSigFileInput.ParsedSigFileInput(fileName,qualifiedNameOfFile,scopedPragmas,hashDirectives,modules))
Some input
| _ ->
seen.Add(qualifiedNameOfFile.Text,Set.singleton path)
input
| input -> input
result,sourceRange,filename,errorLogger.GetErrors ()
with exn ->
System.Diagnostics.Debug.Assert(false, sprintf "unexpected failure in IncrementalFSharpBuild.Parse\nerror = %s" (exn.ToString()))
Expand Down
18 changes: 18 additions & 0 deletions tests/service/ProjectAnalysisTests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -4851,3 +4851,21 @@ let ``Test request for parse and check doesn't check whole project`` () =

()

[<Test>]
// Simplified repro for https://github.com/Microsoft/visualfsharp/issues/2679
let ``add files with same name from different folders`` () =
let fileNames =
[ __SOURCE_DIRECTORY__ + "/data/samename/folder1/a.fs"
__SOURCE_DIRECTORY__ + "/data/samename/folder2/a.fs" ]
let projFileName = __SOURCE_DIRECTORY__ + "/data/samename/tempet.fsproj"
let args = mkProjectCommandLineArgs ("test.dll", fileNames)
let options = checker.GetProjectOptionsFromCommandLineArgs (projFileName, args)
let wholeProjectResults = checker.ParseAndCheckProject(options) |> Async.RunSynchronously
let errors =
wholeProjectResults.Errors
|> Array.filter (fun x -> x.Severity = FSharpErrorSeverity.Error)
if errors.Length > 0 then
printfn "add files with same name from different folders"
for err in errors do
printfn "ERROR: %s" err.Message
shouldEqual 0 errors.Length
5 changes: 5 additions & 0 deletions tests/service/data/samename/folder1/a.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
namespace tempet

module SayA =
let hello name =
printfn "Hello %s" name
5 changes: 5 additions & 0 deletions tests/service/data/samename/folder2/a.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
namespace tempet

module SayB =
let hello name =
printfn "Hello %s" name
13 changes: 13 additions & 0 deletions tests/service/data/samename/tempet.fsproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="FSharp.NET.Sdk;Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

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

Does this test anything valuable? After all it is not using the locally built CompilerService.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it's a NUnit test that loads the proj. I think it doesn't use the sdk at all

<PropertyGroup>
<TargetFramework>netstandard1.6</TargetFramework>
</PropertyGroup>
<ItemGroup>
<Compile Include="folder1/a.fs" />
<Compile Include="folder2/a.fs" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="FSharp.Core" Version="4.1.*" />
<PackageReference Include="FSharp.NET.Sdk" Version="1.0.*" PrivateAssets="All" />
</ItemGroup>
</Project>