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

[FS-1030] - Anonymous records #4499

Merged
merged 37 commits into from Nov 6, 2018

Conversation

Projects
None yet
@dsyme
Contributor

dsyme commented Mar 12, 2018

Replaces #2671

This adds anonymous records to the F# compiler per RFC FS-1030.

Please discuss on the RFC or below - if the discussion gets long we will move to the RFC discussion thread.

The implementation is now complete (apart from copy-and-update) and I believe it to be stable and usable.

Implementation:

  • Equality, hash and comparison on {| X = 1; Y = 2 |} Kind B types
  • ToString on {| X = 1; Y = 2 |} Kind B types
  • sprintf "%A" on {| X = 1; Y = 2 |} Kind B types
  • Quick info
  • Auto complete
  • Highlight symbols
  • F# Interactive multiple fragments
  • Find all references
  • Field ordering is no longer significant
  • Quotations
  • Reflection
  • Additions to Symbols.* FCS API
  • Allow cross assembly references via known types
  • Generate ToString method for better debugging
  • Go-to-definition
  • Copy-and-update
  • suggestions

Testing:

  • Unit testing for language service features
  • LINQ queries
  • Add tests for negative case error messages
  • Add more testing where anon types are mentioned in types but not expressions
  • Add more testing for cross-assembly cases
  • Testing for Symbols.* FCS API

For trial:

  • Test suitability of {| X = 1; Y = 2 |} types for working with known common libraries accepting C# anonymous types (e.g. ASP.NET routing? ML.NET?)

Here is the view of an anonymous value in the VS2017 debugger:

image

If anyone wants to give a hand with the testing that would be great. I think it's really important that someone else besides myself do serious testing on this feature. I'd love someone to try this out and give early feedback on usability and "fit and polish".

@dsyme

This comment has been minimized.

Contributor

dsyme commented Mar 12, 2018

Updated to master, and now F# Interactive now works properly:

> let a = {|  X = 1 |};;
val a : {|X : int|} = {X = 1;}

> a = {|  X = 1 |};;
val it : bool = true

> #q;;

@simontaite simontaite referenced this pull request Mar 13, 2018

Open

Migrations support #2

@majocha

This comment has been minimized.

Contributor

majocha commented Mar 13, 2018

What is the story with type inference?
At the moment stuff like this

let y = {| V = "hi!" |}
let f x = x.V

doesn't work (asks for type annotation) but lambdas are inferred fine. Is this by design?

@smoothdeveloper

This comment has been minimized.

Contributor

smoothdeveloper commented Mar 13, 2018

@majocha I think in the example you give, the type inference will only work with record types.

I don't see why it would infer from anonymous type that happens to be defined in same scope.

Lambdas are infered because they actually carry the type of the expression at their usage spot.

the following type annotation should work:

let y = {| V = "hi!" |}
let f (x: {|V:string|}) = x.V

@dsyme dsyme force-pushed the dsyme:anon-1 branch from 38c5b58 to b952b72 Mar 13, 2018

@dsyme

This comment has been minimized.

Contributor

dsyme commented Mar 13, 2018

@majocha Yes, type annotations are necessary in cases where type information has not flowed to the point of use. This is a major limitation of the feature

You can leave the types incomplete like this

let f (x: {|V: _|} ) = x.V

but there is no row polymorphism. The addition of row-polymorphism for inlined-code could be considered separately

dsyme added some commits Mar 13, 2018

@dsyme

This comment has been minimized.

Contributor

dsyme commented Mar 14, 2018

OK, this feature is now ready. It would be great if someone could give this a full review, and if people could read the RFC very, very carefully.

@Savelenko

This comment has been minimized.

Savelenko commented Mar 14, 2018

  1. The RFC is a bit unclear about adding fields, seemingly putting this out of scope/orthogonal in one section and giving examples in another. So is it or is it not possible to add fields with with?

  2. Why no pattern matching, is there some technical limitation? Is this somehow connected to “no row polymorphism”, as nominal record pattern matching allows leaving out fields? Especially with nested pattern matching this seems like an “ad-hoc” limitation given that other pattern matching forms are so nicely combinable. I think this special case may be quite annoying to many F# programmers and limits expressivity. For example, not being able to do this:

| Some {| X = x |} -> f x
| None -> y
@dsyme

This comment has been minimized.

Contributor

dsyme commented Mar 15, 2018

@Savelenko Thanks for that feedback - I will clarify the RFC and think about pattern matching. You are right that matching a subset of fields is already valid for records and makes sense for anonymous records too. It would not be hard to add.

@dsyme

This comment has been minimized.

Contributor

dsyme commented Mar 15, 2018

@forki

This comment has been minimized.

Contributor

forki commented Mar 15, 2018

image

I personally would prefer if would just print the set difference here.

@isaacabraham

This comment has been minimized.

Contributor

isaacabraham commented Mar 15, 2018

The RFC mentions abbreviations a couple of times with regards to anonymous records - but I couldn't find any examples of how one would actually define such an abbreviation?

Related to this - how will this function with regards to type inference e.g. imagine calling a function foo:

let foo bar =
    bar.X + bar.Y

where bar is an anonymous record created elsewhere (not in scope). What are our options for type hint to tell the compiler that this is an anonymous record? Will the compiler just assume that this is an anonymous type (which would be a change to current behaviour which would be a compiler error)? If not, how would we indicate this to the compiler - like this?

let foo (bar : {| X : int; Y : int |}) =
    bar.X + bar.Y
@forki

This comment has been minimized.

Contributor

forki commented Mar 15, 2018

image

we are missing suggestions here.

@forki

This comment has been minimized.

Contributor

forki commented Mar 15, 2018

Is it expected that we don't have the pipes in ToString() ?

image

@forki

This comment has been minimized.

Contributor

forki commented Mar 15, 2018

image

maybe we should print the field name in this case?

@realvictorprm

This comment has been minimized.

Contributor

realvictorprm commented Mar 15, 2018

@realvictorprm

This comment has been minimized.

Contributor

realvictorprm commented Mar 15, 2018

The CI failes with:

17:16:59 1>    Compiling assembly FSharp.Core, Version=4.4.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:16:59 2>    Compiling assembly System.ValueTuple, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 (CLR v4.0.30319) ...
17:16:59 Failed to load dependency System.Threading.Tasks.Dataflow of assembly Microsoft.Build, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a because of the following error : The system cannot find the file specified. (Exception from HRESULT: 0x80070002)
17:16:59 2>    Compiling assembly System.ValueTuple, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 (CLR v4.0.30319) ...
17:17:03 2>    Compiling assembly FSharp.Core, Version=4.4.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:03 1>    Compiling assembly FSharp.Compiler.Private, Version=10.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:03 3>    Compiling assembly FSharp.Core, Version=4.4.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:03 2>    Compiling assembly D:\j\w\release_ci_pa---866fd2c3\release\net40\bin\FSharp.Build.dll (CLR v4.0.30319) ...
17:17:03 3>    Compiling assembly FSharp.Compiler.Server.Shared, Version=15.6.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:33 1>    Compiling assembly D:\j\w\release_ci_pa---866fd2c3\release\net40\bin\fsc.exe (CLR v4.0.30319) ...
17:17:33 2>    Compiling assembly FSharp.Compiler.Private, Version=10.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:33 2>    Compiling assembly D:\j\w\release_ci_pa---866fd2c3\release\net40\bin\fsi.exe (CLR v4.0.30319) ...
17:17:33 Failed to load dependency System.Threading.Tasks.Dataflow of assembly Microsoft.Build, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a because of the following error : The system cannot find the file specified. (Exception from HRESULT: 0x80070002)
17:17:33 1>    Compiling assembly FSharp.Core, Version=4.4.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:34 2>    Compiling assembly System.ValueTuple, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 (CLR v4.0.30319) ...
17:17:39 1>    Compiling assembly D:\j\w\release_ci_pa---866fd2c3\release\net40\bin\FSharp.Build.dll (CLR v4.0.30319) ...
17:17:39 2>    Compiling assembly FSharp.Core, Version=4.4.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:39 3>    Compiling assembly FSharp.Compiler.Server.Shared, Version=15.6.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:17:39 2>    Compiling assembly FSharp.Compiler.Private, Version=10.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (CLR v4.0.30319) ...
17:18:15 1>    Compiling assembly D:\j\w\release_ci_pa---866fd2c3\release\net40\bin\fsiAnyCpu.exe (CLR v4.0.30319) ...
@gibranrosa

This comment has been minimized.

Contributor

gibranrosa commented Mar 15, 2018

@isaacabraham some posts before @dsyme has already clarified this:
"@majocha Yes, type annotations are necessary in cases where type information has not flowed to the point of use. This is a major limitation of the feature

You can leave the types incomplete like this

let f (x: {|V: _|} ) = x.V"

@isaacabraham

This comment has been minimized.

Contributor

isaacabraham commented Mar 15, 2018

@gibranrosa cheers, I thought as much, but wasn't 100% sure what "abbreviations" meant though :-)

@dsyme

This comment has been minimized.

Contributor

dsyme commented Mar 15, 2018

@dotnet-bot Test Windows_NT Release_ci_part2 Build please

@dsyme

This comment has been minimized.

Contributor

dsyme commented Oct 24, 2018

Odd failure in CI run above when previous run had been green, I've not seen this fail before and seems unrelated:

2018-10-24T11:55:58.1320022Z 1) Failed : Tests.LanguageService.Script.UsingMSBuild.TypeProvider.Disposal.SmokeTest2
2018-10-24T11:55:58.1320127Z   Check3, countCreations() = i, iteration 43
2018-10-24T11:55:58.1320189Z   Expected: True
2018-10-24T11:55:58.1320316Z   But was:  False
2018-10-24T11:55:58.1320540Z at Tests.LanguageService.Script.UsingMSBuild.TypeProviderDisposalSmokeTest(Boolean clearing)
2018-10-24T11:55:58.1320647Z at Tests.LanguageService.Script.UsingMSBuild.TypeProvider.Disposal.SmokeTest2()
@dsyme

This comment has been minimized.

Contributor

dsyme commented Oct 29, 2018

@cartermp Should we go ahead with this? I'd be happy not to have to maintain the branch any more :)

@cartermp

This comment has been minimized.

Collaborator

cartermp commented Oct 29, 2018

I'd like to get one last @KevinRansom checkoff but otherwise yes we should get this in so we can flight it in previews.

@alfonsogarciacaro

This comment has been minimized.

Contributor

alfonsogarciacaro commented Oct 30, 2018

Please move this quickly to FCS when it's merged so we can see how anonymous records work in Fable and give our feedback :)

@forki

This comment has been minimized.

Contributor

forki commented Oct 30, 2018

@ncave

This comment has been minimized.

Contributor

ncave commented Oct 30, 2018

@forki Needs to trickle down to FCS first in order for Fable to use it, hopefully soon after it's merged.

@forki

This comment has been minimized.

Contributor

forki commented Oct 30, 2018

@dsyme

This comment has been minimized.

Contributor

dsyme commented Oct 30, 2018

@ncave @forki If someone could set up auto-integrations from Microsoft/visualfsharp master to FCS that would be amazing :)

(note FCS repo still has more testing of FCS on various Monos, one of the main reasons we don't just release the package here)

@dsyme dsyme closed this Oct 31, 2018

@dsyme dsyme reopened this Oct 31, 2018

@cartermp

This comment has been minimized.

Collaborator

cartermp commented Nov 6, 2018

@TIHan @KevinRansom can we get a review here? I really want to get this into 16.0 preview 2.

@cartermp cartermp requested review from KevinRansom and TIHan Nov 6, 2018

@KevinRansom KevinRansom merged commit e709f52 into Microsoft:master Nov 6, 2018

2 checks passed

license/cla All CLA requirements met.
Details
visualfsharp-CI #20181031.7 succeeded
Details

@cartermp cartermp added this to the 16.0 milestone Nov 6, 2018

@forki

This comment has been minimized.

Contributor

forki commented Nov 6, 2018

wow!!! it's merged. It's in. Can't believe it. @ncave @alfonsogarciacaro did you see that?

micheal1

@forki

This comment has been minimized.

Contributor

forki commented Nov 6, 2018

ok one is not enough.

micheal2

@cartermp

This comment has been minimized.

Collaborator

cartermp commented Nov 6, 2018

This should also be in nightlies for VS shortly. We'd really appreciate further validation work, as we do have some runway with upcoming VS 2019 previews to fix things prior to releasing a new language version.

@baronfel

This comment has been minimized.

Contributor

baronfel commented Nov 6, 2018

would be great to get a prerelease of FSharp.Compiler.Tools for us non-VS-usable folks to test as well. What's the process there?

@forki

This comment has been minimized.

Contributor

forki commented Nov 6, 2018

@vasily-kirichenko

This comment has been minimized.

Contributor

vasily-kirichenko commented Nov 6, 2018

To test it, we need FSharp.Compiler.Tools built from master.

@boeremak

This comment has been minimized.

boeremak commented Nov 6, 2018

Wow, this is great, I was having an issue where I could really use this feature and now I come to find out it has been merged an hour ago. Way to go guys. :)

@cartermp

This comment has been minimized.

Collaborator

cartermp commented Nov 10, 2018

@vasily-kirichenko Feel free to build FCT out of master.

@forki you can build FCS out of master today - AFAIK this is what @Krzysztof-Cieslak does for some Ionide releases.

We'll pull in other PRs related for an F# 4.6 and rev the versions of all assets so we can start shipping an F# 4.6 preview through the .NET CLI as well.

@vasily-kirichenko

This comment was marked as off-topic.

Contributor

vasily-kirichenko commented Nov 10, 2018

good trolling

@dsyme

This comment has been minimized.

Contributor

dsyme commented Nov 13, 2018

As we've discussed before in a few places it is fairly difficult to roll out updates to all of the required tooling for all editor stacks instantly, even for master branch. FCT (fsharp/fsharp) and FCS (fsharp/FSharp.Compiler.Service) are built and released from master but I only tend to do that once the corresponding VS release corresponding to that master is near final.

But I understand why it would be great to roll these out immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment