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 32: add general check for escaping typars to check phase #442

Closed
wants to merge 4 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented May 12, 2015

This (partly) addresses the class of conditions like #32 which give "undefined type variable" internal errors by adding an earlier check to check.fs which gives a user-visible error in this case.

These conditions can in some cases indicate problems with type inference, though in the case of #32 the check is actually the best way to detect the escape condition.

Output from the neg91.fs needs to be updated.

@dsyme
Copy link
Contributor Author

dsyme commented May 12, 2015

A full test pass will be needed to check that this error doesn't trigger on any existing known code.

@latkin
Copy link
Contributor

latkin commented May 18, 2015

I'm seeing two tests that previously passed, and are now failing with the new error. Both have a non-trivial amount of code, so I won't paste it here.

  • tests\fsharpqa\source\Conformance\TypesAndTypeConstraints\TypeParameterDefinitions\ValueTypesWithConstraints01.fs
  • tests\fsharpqa\source\Misc\ConstraintSolverRecursion01.fs

Just compile each source file with no flags to repro.

@dsyme
Copy link
Contributor Author

dsyme commented May 19, 2015

Thanks, I'll take a look.

@dsyme
Copy link
Contributor Author

dsyme commented May 19, 2015

I've updated this - thanks. This line is the relevant addition: dsyme@3c3708f#diff-7044d4e85f35657f78a245e763e92ddcR146. I'm glad we flushed out that particular problem.

Another full test run is needed I'm afraid, sorry about that. it's just one of those things.

Cheers!
Don

@latkin
Copy link
Contributor

latkin commented May 20, 2015

Ok, tests are all green now. GIven that, do you feel this is a safe enough fix that we can apply it this late?

@dsyme
Copy link
Contributor Author

dsyme commented May 20, 2015

I'm OK with putting this off to vNext (e.g. first OOB)

@dsyme dsyme added the V.Next label May 20, 2015
@latkin latkin added this to the F# 4.0 Update 1 milestone Aug 4, 2015
@latkin latkin removed the V.Next label Aug 4, 2015
dsyme added a commit that referenced this pull request Aug 7, 2015
This (partly) addresses the class of conditions like #32
which give "undefined type variable" internal errors by adding
an earlier check to check.fs which gives a user-visible error in
this case.

These conditions can in some cases indicate problems with type
inference, though in the case of #32 the check is actually the
best way to detect the escape condition.

fixes #32
closes #442

commit 5d4d818
Merge: 3c3708f 0b88185
Author: Don Syme <[email protected]>
Date:   Tue May 19 14:33:37 2015 +0100

    merge fsharp4

commit 3c3708f
Author: Don Syme <[email protected]>
Date:   Tue May 19 14:31:06 2015 +0100

    normalzie equi-recursive

commit 220131b
Author: Don Syme <[email protected]>
Date:   Tue May 12 15:32:07 2015 +0100

    update tests, change warning to error

commit 53153b8
Author: Don Syme <[email protected]>
Date:   Tue May 12 15:01:44 2015 +0100

    add general check for escaping typars to check phase
@latkin
Copy link
Contributor

latkin commented Aug 7, 2015

Applied to the OOB branch, thanks!

@latkin latkin closed this Aug 7, 2015
@latkin latkin added the fixed label Aug 7, 2015
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.

3 participants