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

Fix #568: recognize provided expressions #620

Merged
merged 7 commits into from
Aug 12, 2016
Merged

Conversation

Jand42
Copy link
Contributor

@Jand42 Jand42 commented Aug 2, 2016

Expression API recognizes many calls generated by type providers that previously thrown an exception.

Changes included:

  • Add a case TyparRigidity.Unresolved to notate type parameters with unknown type constraints. These are ignored when doing signature comparison checks
  • An unscoping for looking up IL members, version information can be missing from the reference coming from a TP
  • Better recursive stripping of type aliases and measures
  • Added various tests, TestTP is now included in main solution

One TP test method ChoiceConstructionAndMatch fails during F# type checking, union generated subclass fails to resolve to F# symbol, this test is currently commented out

@Jand42
Copy link
Contributor Author

Jand42 commented Aug 2, 2016

Still not correct for use with FSharp.Data+WebSharper 4, adding more tests

@dsyme
Copy link
Contributor

dsyme commented Aug 2, 2016

@Jand42 Wow, impressive

I need to look very closely at the changes in tast.fs and TastOps.fs - if there are any changes there that you think are unnecessary then taking them out would simplify accepting this

One compilation problem:

[00:06:05] C:\projects\fsharp-compiler-service\src\fsharp\ConstraintSolver.fs(348,11): error FS0025: Incomplete pattern matches on this expression. For example, the value '(_,Unresolved)' may indicate a case not covered by the pattern(s).

@Jand42
Copy link
Contributor Author

Jand42 commented Aug 3, 2016

@dsyme Probably all changes in tast.fs and TastOps.fs can be reverted. I am adding more tests to gather and print all method signatures, not just the shape of the expression, I have already found some mistakes.

@Jand42
Copy link
Contributor Author

Jand42 commented Aug 3, 2016

@dsyme I have reverted every change to tast.fs and TastOps.fs, they were not needed.

Added a printer for method reference signatures, and tests with it.

One limitation remains, that if overloads are created with using the same name in CompiledName, it cannot be resolved.

@dsyme
Copy link
Contributor

dsyme commented Aug 5, 2016

Oh that/s great, makes it much easier.

There's still a failure in CI build

[00:08:34] C:\Program Files\dotnet\dotnet.exe compile-fsc @C:\projects\fsharp-compiler-service\tests\service\obj\Release\netcoreapp1.0\dotnet-compile.rsp returned Exit Code 1
[00:08:34] 
[00:08:34] C:\projects\fsharp-compiler-service\tests\service\ExprTests.fs(863,9): error FS0039: The namespace or module 'ProjectCracker' is not defined
[00:08:34] 

None

// Otherwise try to bind it to an F# symbol
match try1 with
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you extract this large expression out to a separate helper function please, outside the big let rec? Along the lines of bindILMethodRefToSymbol in the previous code. Thanks

@dsyme
Copy link
Contributor

dsyme commented Aug 8, 2016

Still seeing the following fail for .NET Core:

[00:08:16] C:\projects\fsharp-compiler-service\tests\service\ExprTests.fs(865,9): error FS0039: The namespace or module 'ProjectCracker' is not defined

@Jand42
Copy link
Contributor Author

Jand42 commented Aug 8, 2016

I have disabled runnint TP tests on .NET core for now.

Added some tests and fixes for accessing module members and get_Tag of non-nullable unions.

I have looked into separating whole ConvILCall or just the inner helper functions, but it would just make it more convoluted with I think. The difference is that old implementation only returned an FSharpMemberOrFunctionOrValue and now it constructs different kinds of expressions, so it can't be on the top of the file without returning a union result which had to be matched to create the final expression. Also, ConvLValueExpr is used depending on the kind of F# member matched, otherwise an extra AddressOf would remain in the expression.

@dsyme
Copy link
Contributor

dsyme commented Aug 9, 2016

OK. Great work. Do you think this is now ready to merge?

@Jand42
Copy link
Contributor Author

Jand42 commented Aug 9, 2016

I will do some testing with it today on FSharp.Data+WebSharper 4, will report back.

@Jand42
Copy link
Contributor Author

Jand42 commented Aug 11, 2016

Everything worked ok (building full WebSharper 4 stack, testing try.websharper), I hope to release all very soon

@dsyme dsyme merged commit 48a703f into fsharp:master Aug 12, 2016
rojepp pushed a commit to rojepp/FSharp.Compiler.Service that referenced this pull request Aug 12, 2016
@dsyme
Copy link
Contributor

dsyme commented Aug 12, 2016

6.0.2 released. Awesome job fixing this!!!

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.

2 participants