Deduplicate module names #2728
Deduplicate module names #2728
Conversation
|
@alfonsogarciacaro You can always say |
|
@alfonsogarciacaro you can run those failing tests locally, no need to wait long time until CI finishes. |
alfonsogarciacaro
changed the title
|
@cartermp @vasily-kirichenko The build script refuses to run the tests locally for me.
|
|
@alfonsogarciacaro Install NUnit 3 test runner adapter VS extension, find a failing test in test explorer and run it. |
|
tests now work on my machine. |
|
@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:
|
|
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 |
|
A couple of notes:
|
|
@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 |
|
@dsyme Awesome, thank you! |
alfonsogarciacaro
changed the title
|
OK. I think now Impl and Sig files are synced and deduplicated. Ready for review |
|
Open Question: do we need to do the same for fsi? |
src/absil/ilwrite.fs
Outdated
There was a problem hiding this comment.
failwithf maybe?
src/absil/ilwrite.fs
Outdated
src/fsharp/ast.fs
Outdated
There was a problem hiding this comment.
I find the old code more compact and readable.
src/fsharp/fsc.fs
Outdated
There was a problem hiding this comment.
Other unused fields are ignored with _, so should _fileName.
src/fsharp/fsc.fs
Outdated
There was a problem hiding this comment.
It's unclear what type this seen thing has. Could you add a comment or, at least, types?
src/fsharp/fsc.fs
Outdated
There was a problem hiding this comment.
These two branches have a lot in common. Maybe it's worth to extract some logic in a function or two?
There was a problem hiding this comment.
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
src/fsharp/fsc.fs
Outdated
There was a problem hiding this comment.
Same, just use _.
src/fsharp/fsc.fs
Outdated
There was a problem hiding this comment.
Why nested try..with? Is it for handling possible exceptions thrown by error(Error(FSComp.SR.fscProblemWritingBinary(outfile, msg), rangeCmdArgs))?
There was a problem hiding this comment.
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)
|
It's hard to review because of lots of whitespace changes. |
|
As written in slack I will revert all changes that are unrelated. Stay
tuned
Am 31.03.2017 14:11 schrieb "Vasily Kirichenko" <[email protected]>:
… 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>
.
|
|
@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 |
|
My approach is to try to not break backwards compatibility. ;-) |
src/fsharp/ast.fs
Outdated
src/fsharp/fsc.fs
Outdated
There was a problem hiding this comment.
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)
src/fsharp/fsc.fs
Outdated
There was a problem hiding this comment.
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
Agreed. |
|
@alfonsogarciacaro You say you need this for Fable 1.0 - I presume you need it integrated to FCS? thanks |
|
@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. |
|
@alfonsogarciacaro Please submit just the fix to FCS. Don't worry about the test |
|
@alfonsogarciacaro Kevin |
7 participants
Giving it a shot at continuing #2686. Apparently I cannot run the tests locally so let see what CI says.