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

[WIP] ConstraintSolver internal error fix #5583

Closed
wants to merge 3 commits into from

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Sep 5, 2018

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

cartermp commented Sep 5, 2018

cc @gusty

@gusty
Copy link
Contributor

gusty commented Sep 5, 2018

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

TIHan commented Sep 5, 2018

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

cartermp commented Sep 5, 2018

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

gusty commented Sep 5, 2018

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

cartermp commented Sep 5, 2018

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

TIHan commented Sep 6, 2018

(b) is most likely what we will do.

@KevinRansom
Copy link
Member

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

TIHan commented Sep 13, 2018

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

gusty commented Apr 18, 2019

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

@brettfo
Copy link
Member

brettfo commented Apr 19, 2019

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

gusty commented May 29, 2019

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