Deduplicate module names #2728

alfonsogarciacaro · 2017-03-29T14:34:56Z

Giving it a shot at continuing #2686. Apparently I cannot run the tests locally so let see what CI says.

cartermp · 2017-03-29T22:15:29Z

@alfonsogarciacaro You can always say @dotnet-bot test this please to trigger CI. No need to make a commit to re-trigger 😄

vasily-kirichenko · 2017-03-30T04:58:58Z

@alfonsogarciacaro you can run those failing tests locally, no need to wait long time until CI finishes.

alfonsogarciacaro · 2017-03-30T12:46:42Z

@cartermp @vasily-kirichenko The build script refuses to run the tests locally for me.

vasily-kirichenko · 2017-03-30T12:51:17Z

@alfonsogarciacaro Install NUnit 3 test runner adapter VS extension, find a failing test in test explorer and run it.

forki · 2017-03-30T14:28:11Z

tests now work on my machine.

forki · 2017-03-30T18:26:55Z

@alfonsogarciacaro it looks it's one step further. It can't write down the IL because the module is twice in the ILTypeDef list

@dsyme it's failing at:

forki · 2017-03-30T19:01:13Z

I think the whole approach is wrong. These implicit modules need to get different internal names. I think we discussed that already somewhere. I will try to look deeper tomorrow. But the qname sorter is definitely too late and fixes only symptoms

alfonsogarciacaro · 2017-03-30T21:13:35Z

A couple of notes:

I can reproduce this with Fable 0.7 (which uses FCS 9.0.1) so it doesn't seem to be a regression
I cannot reproduce it always when there are two files with same name, I think it's because the problem comes from CanonicalizeFilename which is only called for files which have a single module and other conditions I don't understand :/

dsyme · 2017-03-30T23:31:17Z

@alfonsogarciacaro I'll wirk on this first thing in the morning, followed by the stack overflow issue.

I understand now how important this is for Fable 1.0

alfonsogarciacaro · 2017-03-30T23:38:47Z

@dsyme Awesome, thank you!

forki · 2017-03-31T09:13:12Z

I think I fixed it with this deduplication trick: https://github.com/alfonsogarciacaro/visualfsharp/pull/4/commits/7fcc07d50ade37f9e3269c73dc77ecc48309343c#diff-d58c6e25aebd2ff64de33a050b0228ccR1766

forki · 2017-03-31T11:48:14Z

OK. I think now Impl and Sig files are synced and deduplicated. Ready for review

forki · 2017-03-31T11:50:24Z

Open Question: do we need to do the same for fsi?

vasily-kirichenko · 2017-03-31T11:55:00Z

src/absil/ilwrite.fs

@@ -471,7 +471,8 @@ type MetadataTable<'T> =
        t |> Array.iteri (fun i x -> h.[x] <- (i+1))

    member tbl.AddUniqueEntry nm geterr x =
-        if tbl.dict.ContainsKey x then failwith ("duplicate entry '"+geterr x+"' in "+nm+" table")
+        if tbl.dict.ContainsKey x then 
+            failwith ("duplicate entry '" + geterr x + "' in " + nm + " table")


failwithf maybe?

I just changed alignment

vasily-kirichenko · 2017-03-31T11:55:47Z

src/absil/ilwrite.fs

@@ -3547,7 +3548,7 @@ let writeBinaryAndReportMappings (outfile, ilg: ILGlobals, pdbfile: string optio
    // Store the public key from the signer into the manifest.  This means it will be written 
    // to the binary and also acts as an indicator to leave space for delay sign 

-    reportTime showTimes "Write Started";
+    reportTime showTimes "Write Started"


vasily-kirichenko · 2017-03-31T12:01:27Z

src/fsharp/ast.fs

+
+    member x.Range = 
+        let (QualifiedNameOfFile t) = x
+        t.idRange


I find the old code more compact and readable.

Agreed on this one

vasily-kirichenko · 2017-03-31T12:02:14Z

src/fsharp/fsc.fs

@@ -303,7 +303,7 @@ module InterfaceFileWriter =
            fprintfn os "#light" 
            fprintfn os "" 

-        for (TImplFile(_, _, mexpr, _, _)) in declaredImpls do
+        for (TImplFile(_fileName, _, _, mexpr, _, _)) in declaredImpls do


Other unused fields are ignored with _, so should _fileName.

vasily-kirichenko · 2017-03-31T12:03:54Z

src/fsharp/fsc.fs

+
+    let inputs =
+        // Deduplicate module names
+        let seen = Dictionary<_,Set<_>>()


It's unclear what type this seen thing has. Could you add a comment or, at least, types?

vasily-kirichenko · 2017-03-31T12:07:45Z

src/fsharp/fsc.fs

+                    let qualifiedNameOfFile = if count = 1 then qualifiedNameOfFile else QualifiedNameOfFile(Ident(id.idText + "___" + count.ToString(),id.idRange))
+                    let input = ParsedInput.SigFile (ParsedSigFileInput.ParsedSigFileInput(fileName,qualifiedNameOfFile,scopedPragmas,hashDirectives,modules))
+                    input,x
+                | _ ->


These two branches have a lot in common. Maybe it's worth to extract some logic in a function or two?

Yeah, my preference is to encapsulate such name generators (though I haven't done it particularly well - see the ones in ast.fs which are not suitable here since they use range line number information). So

type UniqueNameGenerator() = let seen = Dictionary... member x.GetName(text) = match seen.TryGetValue text with | true, paths -> let count = if paths.Contains path then paths.Count else paths.Count + 1 seen.[text] <- Set.add path paths if count = 1 then text else text + "___" + count.ToString()

etc

vasily-kirichenko · 2017-03-31T12:08:04Z

src/fsharp/fsc.fs

@@ -1812,7 +1846,7 @@ let main1(Args (ctok, tcGlobals, tcImports: TcImports, frameworkTcImports, gener
    // it as the updated global error logger and never remove it
    let oldLogger = errorLogger
    let errorLogger = 
-        let scopedPragmas = [ for (TImplFile(_, pragmas, _, _, _)) in typedImplFiles do yield! pragmas ]
+        let scopedPragmas = [ for (TImplFile(_fileName, _, pragmas, _, _, _)) in typedImplFiles do yield! pragmas ]


Same, just use _.

vasily-kirichenko · 2017-03-31T12:09:38Z

src/fsharp/fsc.fs

+            error(Error(FSComp.SR.fscProblemWritingBinary(outfile, msg), rangeCmdArgs))
+    with e -> 
+        errorRecoveryNoRange e
+        exiter.Exit 1 


Why nested try..with? Is it for handling possible exceptions thrown by error(Error(FSComp.SR.fscProblemWritingBinary(outfile, msg), rangeCmdArgs))?

He's just changing the indenting in this case ( @forki - perhaps remove the formatting changes to make the PR minimal? I don't mind either way though)

vasily-kirichenko · 2017-03-31T12:11:11Z

It's hard to review because of lots of whitespace changes.

forki · 2017-03-31T12:12:34Z

As written in slack I will revert all changes that are unrelated. Stay tuned Am 31.03.2017 14:11 schrieb "Vasily Kirichenko" <notifications@github.com>:

…

It's hard to review because of lots of whitespace changes. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2728 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNBNVvQBNHihKofKtkOLpdXX7aW30ks5rrO1fgaJpZM4MtEnP> .

dsyme · 2017-03-31T12:14:37Z

@forki Stepping back a bit, this whole QualifiedNameOfFile stuff is really irritating. AFAICS it's only used for the names given to these internally generated classes:

That's all. Just a simple name. So easy. I believe the only issue is that name must match between the processing of the signature of implementation - because other files are allowed to access straight into these items - e.g. when taking the address of a mutable static fields.

So on a quick glance your approach is basically right - generate nice names but be careful about de-duplicating. That said, there's probably a much easier way to do this all (the whole thing, not just the de-duplicating) given the end result is so simple, or even just to somehow get rid of the information altogether

forki · 2017-03-31T12:17:00Z

My approach is to try to not break backwards compatibility. ;-)

dsyme

minor changes requested

dsyme · 2017-03-31T12:15:59Z

src/fsharp/ast.fs

+
+    member x.Range = 
+        let (QualifiedNameOfFile t) = x
+        t.idRange


Agreed on this one

dsyme · 2017-03-31T12:18:22Z

src/fsharp/fsc.fs

+            error(Error(FSComp.SR.fscProblemWritingBinary(outfile, msg), rangeCmdArgs))
+    with e -> 
+        errorRecoveryNoRange e
+        exiter.Exit 1 


He's just changing the indenting in this case ( @forki - perhaps remove the formatting changes to make the PR minimal? I don't mind either way though)

dsyme · 2017-03-31T12:24:44Z

src/fsharp/fsc.fs

+                    let qualifiedNameOfFile = if count = 1 then qualifiedNameOfFile else QualifiedNameOfFile(Ident(id.idText + "___" + count.ToString(),id.idRange))
+                    let input = ParsedInput.SigFile (ParsedSigFileInput.ParsedSigFileInput(fileName,qualifiedNameOfFile,scopedPragmas,hashDirectives,modules))
+                    input,x
+                | _ ->


Yeah, my preference is to encapsulate such name generators (though I haven't done it particularly well - see the ones in ast.fs which are not suitable here since they use range line number information). So

type UniqueNameGenerator() = let seen = Dictionary... member x.GetName(text) = match seen.TryGetValue text with | true, paths -> let count = if paths.Contains path then paths.Count else paths.Count + 1 seen.[text] <- Set.add path paths if count = 1 then text else text + "___" + count.ToString()

etc

dsyme · 2017-03-31T12:26:09Z

My approach is to try to not break backwards compatibility. ;-)

Agreed.

dsyme · 2017-03-31T12:26:43Z

@alfonsogarciacaro You say you need this for Fable 1.0 - I presume you need it integrated to FCS? thanks

forki

Cool! So how do we get that into FCS? ;-)

alfonsogarciacaro · 2017-03-31T13:14:24Z

@forki has removed the whitespace changes and I've tried to implement some of the comments, please review.

@dsyme Yes, this would be needed for FCS. I can send a PR there but I guess it's no use, as changes in this repo will flow to FCS anyway. It would be ideal if a new FCS version is pushed to nuget, but it it's cumbersome to do it at this moment, I can just fork it and add the change manually for the time being.

dsyme · 2017-03-31T13:58:22Z

@alfonsogarciacaro Please submit just the fix to FCS. Don't worry about the test

KevinRansom · 2017-03-31T18:50:51Z

@alfonsogarciacaro
Thank you for this

Kevin

msftclas added the cla-not-required label Mar 29, 2017

alfonsogarciacaro mentioned this pull request Mar 29, 2017

Error: An implementation of file has already been given fable-compiler/Fable#781

Closed

cartermp requested a review from dsyme March 29, 2017 22:15

alfonsogarciacaro mentioned this pull request Mar 30, 2017

Same file name across different folders fails the compilation with error FS0239 #2679

Closed

alfonsogarciacaro changed the title ~~CompileOps.qnameOrder uses full file path without extension to compare instead of module name inferred from file name~~ [WIP] CompileOps.qnameOrder uses full file path without extension to compare instead of module name inferred from file name Mar 30, 2017

alfonsogarciacaro mentioned this pull request Mar 30, 2017

StackOverflowException when compiling with FSharp.Compiler.Tools 4.1.1 #2682

Closed

alfonsogarciacaro changed the title ~~[WIP] CompileOps.qnameOrder uses full file path without extension to compare instead of module name inferred from file name~~ Deduplicate module names Mar 31, 2017

vasily-kirichenko reviewed Mar 31, 2017

View reviewed changes

dsyme requested changes Mar 31, 2017

View reviewed changes

alfonsogarciacaro added 2 commits March 31, 2017 14:51

Deduplicate module names

52652a2

Small refactor of deduplicate module name operation

a9b8660

forki approved these changes Mar 31, 2017

View reviewed changes

dsyme approved these changes Mar 31, 2017

View reviewed changes

alfonsogarciacaro mentioned this pull request Mar 31, 2017

Deduplicate module names fsharp/fsharp-compiler-docs#748

Merged

KevinRansom approved these changes Mar 31, 2017

View reviewed changes

KevinRansom merged commit f6e160a into dotnet:master Mar 31, 2017

This was referenced Apr 1, 2017

[WIP] Add test for #748 fsharp/fsharp-compiler-docs#749

Merged

Deduplicate module names in FCS #2772

Merged

Deduplicate module names #2728

Deduplicate module names #2728

Conversation

alfonsogarciacaro commented Mar 29, 2017

cartermp commented Mar 29, 2017

vasily-kirichenko commented Mar 30, 2017

alfonsogarciacaro commented Mar 30, 2017 • edited Loading

vasily-kirichenko commented Mar 30, 2017

forki commented Mar 30, 2017

forki commented Mar 30, 2017

forki commented Mar 30, 2017

alfonsogarciacaro commented Mar 30, 2017

dsyme commented Mar 30, 2017

alfonsogarciacaro commented Mar 30, 2017

forki commented Mar 31, 2017

forki commented Mar 31, 2017

forki commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasily-kirichenko commented Mar 31, 2017

forki commented Mar 31, 2017 via email

dsyme commented Mar 31, 2017

forki commented Mar 31, 2017

dsyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Mar 31, 2017

dsyme commented Mar 31, 2017

forki left a comment • edited Loading

Choose a reason for hiding this comment

alfonsogarciacaro commented Mar 31, 2017

dsyme commented Mar 31, 2017

KevinRansom commented Mar 31, 2017

alfonsogarciacaro commented Mar 30, 2017 •

edited

Loading

forki left a comment •

edited

Loading