Skip to content

[WIP] ConstraintSolver internal error fix#5583

Closed
TIHan wants to merge 3 commits into
dotnet:dev15.9from
TIHan:internal-error-fix2
Closed

[WIP] ConstraintSolver internal error fix#5583
TIHan wants to merge 3 commits into
dotnet:dev15.9from
TIHan:internal-error-fix2

Conversation

@TIHan

@TIHan TIHan commented Sep 5, 2018

Copy link
Copy Markdown
Contributor

Resolving this issue: #5580

Should not regress: #4343 (comment)

@TIHan TIHan changed the title [WIP] ConstrainSolver internal error fix [WIP] ConstraintSolver internal error fix Sep 5, 2018
@cartermp

cartermp commented Sep 5, 2018

Copy link
Copy Markdown
Contributor

cc @gusty

@gusty

gusty commented Sep 5, 2018

Copy link
Copy Markdown
Contributor

I'm more than happy to see this reverted.

@TIHan What's the difference between this reversion and the one I attempted in #4173, in order to keep #4343 working?

@TIHan

TIHan commented Sep 5, 2018

Copy link
Copy Markdown
Contributor Author

@gusty this one is slightly different, instead of returning the Locally type, I just return the UnresolvedOverloading error in SolveMemberConstraint. That seems to fix the issue and not regress, but it seems it is also breaking other stuff.

This one is going to be very tricky. It seems even a slight tweak will change behavior in a way that isn't expected.

@cartermp

cartermp commented Sep 5, 2018

Copy link
Copy Markdown
Contributor

@dsyme could comment further, but I suspect that if we cannot derive a fix for the internal error without breaking some kind of SRTP behavior, we'll either:

(a) Never fix it, thus keeping this wart here forever
(b) Make this an intentional breaking change, ship it in F# 5.0, and document all workarounds we know about

@gusty

gusty commented Sep 5, 2018

Copy link
Copy Markdown
Contributor

I know it's gonna be tricky, the way I see it is that if it's not possible to fix it in such a way (b) will be the only option.

@cartermp

cartermp commented Sep 5, 2018

Copy link
Copy Markdown
Contributor

IIRC the break that eventually caused a re-reversion has had that project undergo changes so that they weren't dependent on the broken behavior, so I do lean towards (b), since it is a major language change where we'll be doing other "invasive" things (Nullable References).

@TIHan

TIHan commented Sep 6, 2018

Copy link
Copy Markdown
Contributor Author

(b) is most likely what we will do.

@KevinRansom

Copy link
Copy Markdown
Contributor

@TIHan

Is this still WIP? It looks okay to me. It's targeting the de15.9 branch and so we need to prioritize getting it done. OR are we going to instead target this at dev16.0?

If we are aiming at 15.9 do we have a bug in TFS for tracking this?

/cc @cartermp, @Pilchie, @dsyme

@TIHan

TIHan commented Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

This is still WIP and will not be in dev15.9. It could have been if the fix was quick and didn't break other stuff; but unfortunately it did. The correct fix will take considerable more time to figure out.

@gusty

gusty commented Apr 18, 2019

Copy link
Copy Markdown
Contributor

Why was this closed?
Is this issue solved somewhere else or was finally decided to revert the offending PR?

@brettfo

brettfo commented Apr 19, 2019

Copy link
Copy Markdown
Member

Looks like it was auto-closed when I cleaned up the dev15.9 branch; sorry about that. You should be able to rebase this onto master and re-open or re-create.

@gusty

gusty commented May 29, 2019

Copy link
Copy Markdown
Contributor

Since this was not solved, can we revert again to the correct behavior and use the language switch for F# 4.7?

@cartermp

Copy link
Copy Markdown
Contributor

@gusty or @TIHan could you re-open this and target master?

I agree that with LangVersion in we can reconsider a fix like 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.

5 participants