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

[FS-1030] - Anonymous records #4499

Merged
merged 37 commits into from
Nov 6, 2018
Merged

[FS-1030] - Anonymous records #4499

merged 37 commits into from
Nov 6, 2018

Conversation

dsyme
Copy link
Contributor

@dsyme 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
Copy link
Contributor Author

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;;

@majocha
Copy link
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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

  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
Copy link
Contributor Author

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
Copy link
Contributor Author

dsyme commented Mar 15, 2018

@Savelenko See fsharp/fslang-design#265, thanks

@forki
Copy link
Contributor

forki commented Mar 15, 2018

image

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

@isaacabraham
Copy link
Contributor

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
Copy link
Contributor

forki commented Mar 15, 2018

image

we are missing suggestions here.

@forki
Copy link
Contributor

forki commented Mar 15, 2018

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

image

@forki
Copy link
Contributor

forki commented Mar 15, 2018

image

maybe we should print the field name in this case?

@realvictorprm
Copy link
Contributor

realvictorprm commented Mar 15, 2018 via email

@realvictorprm
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

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

@dsyme
Copy link
Contributor Author

dsyme commented Mar 15, 2018

@dotnet-bot Test Windows_NT Release_ci_part2 Build please

@dsyme dsyme reopened this Oct 31, 2018
@cartermp
Copy link
Contributor

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 November 6, 2018 17:10
@KevinRansom KevinRansom merged commit e709f52 into dotnet:master Nov 6, 2018
@cartermp cartermp added this to the 16.0 milestone Nov 6, 2018
@forki
Copy link
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
Copy link
Contributor

forki commented Nov 6, 2018

ok one is not enough.

micheal2

@cartermp
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

forki commented Nov 6, 2018 via email

@vasily-kirichenko
Copy link
Contributor

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

@boeremak
Copy link

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
Copy link
Contributor

@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 has been minimized.

@dsyme
Copy link
Contributor Author

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.

@cmeeren
Copy link
Contributor

cmeeren commented Jan 22, 2019

Has this been released in relative silence? I'm using VS 15.9.5 (no VF# nightlies) and am able to use anonymous records, but I can't find anything about this fantastic feature in any VS release notes.

@cartermp
Copy link
Contributor

@cmeeren If you're using it then you have either a nightly build or a self-built VSIX installed. This was not released yet.

@cmeeren
Copy link
Contributor

cmeeren commented Jan 22, 2019

Hm, strange. Does the information below indicate a version other than the one shipped with VS?

Microsoft Visual Studio Professional 2017 
Version 15.9.5
VisualStudio.15.Release/15.9.5+28307.280
Microsoft .NET Framework
Version 4.7.03056

Installed Version: Professional

...

Visual F# Tools 10.2 for F# 4.5   15.8.0.0.  Commit Hash: ace88e6d3ffaf5e4ada0153a56e78ad0a75e82c8.
Microsoft Visual F# Tools 10.2 for F# 4.5

...

@cartermp
Copy link
Contributor

Check the extensions page (Tools > Extensions and Updates) to see the version there.

@cmeeren
Copy link
Contributor

cmeeren commented Jan 22, 2019

Would you look at that, I did in fact have a nightly installed... 😅😳 I was certain I had uninstalled it long ago. Sorry for the trouble!

@alfonsogarciacaro
Copy link
Contributor

I'm a bit sad about this. Without getting any hint from the F# team and after spending a lot of time to investigate this myself, I've finally realized this has shipped without including anonymous records in the AST exposed by FSharpImplementationFileContents (src/fsharp/symbols/Exprs.fs hasn't be updated except for a couple of lines) so anonymous records remain invisible to Fable. There's only support for symbols in editors 😞

@cartermp
Copy link
Contributor

cartermp commented Feb 1, 2019

@dsyme any idea what it would take to add the relevant case to this so that fsExpr is updated for Fable?

@alfonsogarciacaro Since it's still in preview things aren't quite baked yet. Though even if this were shipped I believe we could update FCS after the fact.

@alfonsogarciacaro
Copy link
Contributor

@cartermp All the signs are the fsharp/FSharp.Compiler.Service repo and the corresponding nuget package are being abandoned. When someone wants to use FCS to test latest features they're encouraged to build it from this repository. However, there's no incentive to maintain the FCS API (beyond the symbols for editor support) when the developing in this repository, as the current situation has shown.

The feeling is that anyone with a project relying on the Expression tree API from FCS should be prepared to become a maintainer of this repository to make sure new features get implemented in the Expression tree too. Something that my already stretched-too-thin time budget for OSS doesn't allow me to do.

@cartermp
Copy link
Contributor

cartermp commented Feb 1, 2019

I acknowledge the current issue is annoying, but it's important to stress that the feature is still in preview and has only ever been advertised as such, so there's time to make sure we emit something Fable can consume before anything is considered completed.

As for the FCS package/repo, we can put together a proposal to publish from this repository and make it a part of our release checklist. We can discuss the issue of maintaining the Expression Tree API as a part of that, if that's something that would be desirable.

@Krzysztof-Cieslak
Copy link
Contributor

As for the FCS package/repo, we can put together a proposal to publish from this repository and make it a part of our release checklist.

I’d rather not have release cycle of FCS coupled with VF#. Don has given NuGet release access to @baronfel so maybe we will see new releases more often. And TBF, we probably just need to write some automated sync process (for example using GitHub actions when they are out of closed preview).

And as of omitting anon records in expression tree... it seems that was just small mistake, no need to make this as example of some deeper problem (https://twitter.com/dsyme/status/1091311874514321408?s=21)

@baronfel
Copy link
Member

baronfel commented Feb 2, 2019

I'd echo what @Krzysztof-Cieslak has said. I'm on top of (and motivated to!) merge FCS with a more regular cadence and I look forward to automating/documenting that flow even moreso than has already been done in that repo. I'm working on making the periodic resyncs of FCS and fsharp/fsharp more painless for the community, but the first step from my perspective to to establish a routine of doing them quickly and painlessly, before automating it all away.

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.