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

[RFC FS-1068] open static classes #6807

Merged
merged 14 commits into from
Jul 9, 2019

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented May 22, 2019

Continuation of #6309

Proof of concept for RFC FS-1068 Open static classes.

Details are on the RFC, but for example:

> open System.Math;;
> Min(1.0, 2.0);;
val it : float = 1.0

> Min(2.0, 1.0);;
val it : float = 1.0

> open System.Math;;
> Min(2.0, 1.0);;
val it : float = 1.0

DONE:

  • Resolve how static classes are defined in F#
  • Test opening classes with AutoOpen attribute
  • Respect and test for RequireQualifiedNameAttribute(true) on a static class, that prevents it being opened, or at least gives a warning, as for F# modules.
  • Check completions on "open System.Ma$"
  • Resolve open v open static

@dsyme dsyme mentioned this pull request May 22, 2019
2 tasks
@cartermp
Copy link
Contributor

@dsyme Do the two TODOs make this a WIP?

@dsyme
Copy link
Contributor Author

dsyme commented May 31, 2019

@cartermp Yes, though it should be very quick to resolve these and close this out.

@dsyme dsyme changed the title [RFC FS-1068] open static classes [WIP, RFC FS-1068] open static classes May 31, 2019
@dsyme dsyme changed the title [WIP, RFC FS-1068] open static classes [RFC FS-1068] open static classes Jun 7, 2019
@cartermp
Copy link
Contributor

cartermp commented Jun 7, 2019

Tagging @TIHan for review

@cartermp cartermp added this to the .NET Core 3.0 milestone Jun 17, 2019
@cartermp cartermp requested a review from TIHan June 27, 2019 21:40
@dsyme
Copy link
Contributor Author

dsyme commented Jul 3, 2019

@cartermp @TIHan We have one remaining design issue for this feature, fsharp/fslang-design#352 (comment), basically open v. open static

@dsyme
Copy link
Contributor Author

dsyme commented Jul 3, 2019

@cartermp @TIHan We have one remaining design issue for this feature, fsharp/fslang-design#352 (comment), basically open v. open static

I have resolved this (fsharp/fslang-design#352 (comment)), and this PR is now ready for review

@KevinRansom @TIHan we need to add language version flag protection for this feature. What's our status for doing this? I'm happy to do it but it needs to be done ASAP

Also should this go into feature/fsharp47 or master? Again happy to retarget if necessary

@dsyme
Copy link
Contributor Author

dsyme commented Jul 3, 2019

@cartermp please re-review when you get the chance

@dsyme
Copy link
Contributor Author

dsyme commented Jul 3, 2019

@KevinRansom @TIHan Please advise on adding the /langversion checks for this PR. This needs to be done asap, thanks. I can do it, just let me know.

@cartermp
Copy link
Contributor

cartermp commented Jul 3, 2019

@dsyme if you have the time, feel free to guard this with the langversion switch without awaiting @KevinRansom. We're still getting our ducks in row for 16.3 :)

@cartermp cartermp dismissed their stale review July 3, 2019 19:18

Addressed

@dsyme dsyme force-pushed the feature/open-static-classes branch from 929698e to e3262b3 Compare July 4, 2019 14:51
@dsyme dsyme force-pushed the feature/open-static-classes branch from e3262b3 to 607fa35 Compare July 4, 2019 14:52
@dsyme dsyme changed the base branch from master to release/fsharp47 July 4, 2019 14:52
@dsyme
Copy link
Contributor Author

dsyme commented Jul 4, 2019

@KevinRansom @cartermp I rebased to release/fsharp47 instead of master

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

I think it looks fine code-wise, just a few minor questions above.

dotnet-maestro and others added 8 commits July 5, 2019 12:57
…704.2

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19354.2
…atic-classes

Merge master to feature/open-static-classes
…705.2

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19355.2
…atic-classes

Merge master to feature/open-static-classes
…706.1

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19356.1
…9c7-035f4f8673df

[master] Update dependencies from dotnet/arcade
@dsyme
Copy link
Contributor Author

dsyme commented Jul 8, 2019

@KevinRansom I resolved the conflicts, you might want to double check

@dsyme
Copy link
Contributor Author

dsyme commented Jul 8, 2019

Ugh I messed up the merge,

C:\GitHub\dsyme\visualfsharp2\tests\FSharp.Core.UnitTests\FSharp.Core\Microsoft.FSharp.Core\NameOfTests.fs(19,22): erro
r FS0039: The value or constructor 'nameof' is not defined. [C:\GitHub\dsyme\visualfsharp2\tests\FSharp.Core.UnitTests\
FSharp.Core.UnitTests.fsproj]
C:\GitHub\dsyme\visualfsharp2\tests\FSharp.Core.UnitTests\FSharp.Core\Microsoft.FSharp.Core\NameOfTests.fs(21,34): erro
r FS0039: The value or constructor 'nameof' is not defined. [C:\GitHub\dsyme\visualfsharp2\tests\FSharp.Core.UnitTests\
FSharp.Core.UnitTests.fsproj]

dsyme and others added 2 commits July 8, 2019 16:56
…atic-classes

Merge master to feature/open-static-classes
@dsyme
Copy link
Contributor Author

dsyme commented Jul 9, 2019

We have a heisen-error in the Linux Source Build:

.packages/microsoft.sourcelink.common/1.0.0-beta2-19351-01/build/InitializeSourceControlInformation.targets(3,81): error MSB4022: The result "" of evaluating the value "$(_MicrosoftSourceLinkCommonAssemblyFile)" of the "AssemblyFile" attribute in element <UsingTask> is not valid.
.packages/microsoft.sourcelink.common/1.0.0-beta2-19351-01/build/InitializeSourceControlInformation.targets(3,81): error MSB4022: The result "" of evaluating the value "$(_MicrosoftSourceLinkCommonAssemblyFile)" of the "AssemblyFile" attribute in element <UsingTask> is not valid.
Build failed (exit code '1').
Bash exited with code '1'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants