Skip to content

[WIP] - Suggestion: Don't implicitly append Module suffix if type with same name has type arguments.#1772

Merged
KevinRansom merged 1 commit intodotnet:masterfrom
kurtschelfthout:module-suffix-fix
Nov 21, 2016
Merged

[WIP] - Suggestion: Don't implicitly append Module suffix if type with same name has type arguments.#1772
KevinRansom merged 1 commit intodotnet:masterfrom
kurtschelfthout:module-suffix-fix

Conversation

@kurtschelfthout
Copy link
Contributor

@kurtschelfthout kurtschelfthout commented Nov 17, 2016

Don't implicitly append 'Module' to module names if the type with the same name as the module has a type argument. In that case the name is not ambiguous.

In addition, the current situation breaks these fairly common use cases for libraries that need to interoperate with other .NET languages, as with the new compiler some modules will suddenly gain a Module suffix.

There is no way to turn this off, short of making the module into a static class, which is silly.

This issue is also discussed in somewhat more detail here: fsharp/fslang-design#108

@kurtschelfthout
Don't append Module for types with arguments.
d0c2abe 
Don't implicitly append 'Module' to module names if the type with the
same name as the module has a type argument. In that case the name is
not ambiguous.
In addition, the current situation breaks these fairly common use cases
for libraries that need to interoperate with other .NET languages, as
with the new compiler some modules will suddenly gain a Module suffix.
There is no way to turn this off, short of making the module into a
static class, which is silly.
@KevinRansom KevinRansom changed the title Suggestion: Don't implicitly append Module suffix if type with same name has type arguments. [WIP] - Suggestion: Don't implicitly append Module suffix if type with same name has type arguments. Nov 17, 2016
@kurtschelfthout
Copy link
Contributor Author

kurtschelfthout commented Nov 18, 2016

Both failures are unrelated false negatives.

Unrelated: this seems to happen regularly, I think this causes a lot of unnecessary friction and lost time for contributors - I will take a look at the race condition in the async module on AppVeyor which is probably timer related separately.

The Ubuntu build fails to restore a bunch of NuGet packages.

@dsyme
Copy link
Contributor

dsyme commented Nov 18, 2016

Looks like a heisen failure:

[00:51:37] 
[00:51:37] 1) Failed : FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.OnCancel.RaceBetweenCancellationHandlerAndDisposingHandlerRegistration
[00:51:37]   Expected: True
[00:51:37]   But was:  False
[00:51:37] at <StartupCode$FSharp-Core-Unittests>.$AsyncModule.test@286-22(Unit unitVar0) in C:\projects\visualfsharp-3dtit\src\fsharp\FSharp.Core.Unittests\FSharp.Core\Microsoft.FSharp.Control\AsyncModule.fs:line 299
[00:51:37] at FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.OnCancel.RaceBetweenCancellationHandlerAndDisposingHandlerRegistration() in C:\projects\visualfsharp-3dtit\src\fsharp\FSharp.Core.Unittests\FSharp.Core\Microsoft.FSharp.Control\AsyncModule.fs:line 300

@kurtschelfthout
Copy link
Contributor Author

kurtschelfthout commented Nov 19, 2016

There is an issue for that test: #1755

Just as an fyi, that the tests are kicked off again when someone comments or edits is a bit mentally jarring. For example, right now this PR is all green so I'm actually wondering whether to actually add this comment because likely something is going to fail when the build is kicked off again :)

As a compromise I decided to take a screenshot as "proof" ;)

image

@kurtschelfthout
Copy link
Contributor Author

kurtschelfthout commented Nov 21, 2016

@KevinRansom why is this WIP? From my perspective no more changes are needed...

@dsyme
Copy link
Contributor

dsyme commented Nov 21, 2016

@KevinRansom @kurtschelfthout I approve this PR - I agree it's a necessary design adjustment/fix to an F# 4.1 feature.

@KevinRansom KevinRansom merged commit 9367586 into dotnet:master Nov 21, 2016
@Booksbaum Booksbaum mentioned this pull request Oct 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kurtschelfthout @dsyme @KevinRansom @msftclas