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] Improve type inference for non-sealed types #7458

Closed
wants to merge 11 commits into from
Closed

[WIP] Improve type inference for non-sealed types #7458

wants to merge 11 commits into from

Conversation

gusty
Copy link
Contributor

@gusty gusty commented Aug 28, 2019

This attempts to solve a longstanding problem where type inference in presence of member constraints is broken, but succeed for sealed types.

A repro will be added as a test.

@cartermp cartermp closed this Aug 28, 2019
@cartermp cartermp reopened this Aug 28, 2019
@dsyme
Copy link
Contributor

dsyme commented Aug 29, 2019

A repro will be added as a test.

Just to mention we'll need much more systematic testing to accept a change in this area.

There's a lot of additional testing in #6805 which will be relevant, but I think you should incorporate bespoke systematic testing across several dimensions of variability.

@gusty
Copy link
Contributor Author

gusty commented Sep 4, 2019

Thanks @dsyme I agree that this would require lot of testing.
Still, I think this is something we need to fix somehow, specially considering the desire to improve SRTP support.

The code I'm testing is from this issue #4171 I just revived with a new repro. Right now, with the code I have so far it compiles fine.

But at the moment this is just a playground to allow me to understand better what's happening and why a non-sealed type makes things bad when we have a backtracking mechanism in place.

Btw, it's kind of funny that the only test this PR breaks at the moment is also my code, so I'm still breaking myself. But talking seriously, this is also an indication that including F#+ tests here would be a good thing, it's a shame this idea was pushed back (CC @cartermp )

@TIHan
Copy link
Contributor

TIHan commented Sep 9, 2019

I'm not sure what the change is doing at the moment, but to comment my thoughts on SRTP changes:

We should be trying to fix flaws in SRTP first before adding more improvements/additions to it. I feel this way because I think that adding more improvements/additions could add even more flaws in SRTP which will not be brought up until years later. I've looked at SRTP issues a few weeks ago and my head was exploding due to how complex it was. Opening it up even more causes me to worry.

I don't know if this change will fall into the "fixing flaw" or "improvement/addition" category because I need to see an example to understand it.

@gusty
Copy link
Contributor Author

gusty commented Sep 10, 2019

@TIHan I agree with you, except that to me this is clearly a flaw, maybe I should change the title to reflect this.

Also, this is something we definitely have to fix as part of the first item in this list: fsharp/fslang-suggestions#243 (comment)

As you say, this is really hard to understand, but I'm doing the effort because I want to see these issues solved for once.

@wallymathieu
Copy link
Contributor

This is clearly a flaw as @gusty mentions. It would be nice to have a bit more consistency.

@KevinRansom
Copy link
Member

@gusty , is this PR still in development? it's been 9 months since the last commit or comment. I am closing this for now, please re-open when you are ready to proceed with it.

Thanks

Kevin

@gusty
Copy link
Contributor Author

gusty commented May 28, 2020

I might do more work in the future.
If so, in principle I'll be interested only in the checks, then only if I ever get something working I would be interested in merging it.
Thanks !

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.

6 participants