Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Jun 15, 2020

While integrating #6810 into #6811 a regression turned up.

The problem lies in the fact that an extra constraint is being asserted during optimization. The relevant lines of #6810 are things like this and [this]. The tril fix removes these lines which were not needed. Extra constraints should not need to be asserted and this addition later in the development of #6810 meant these other two changes are not needed.

TODO:

  • add testing around the regression

Note the regression is active even if langversion is not set, which also indicates some inaccurate coding in #6810

@dsyme dsyme mentioned this pull request Jun 16, 2020
3 tasks
@dsyme dsyme changed the title trial alternative fix for 9449 Fix 9449 Jun 16, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Jun 16, 2020

Unfortunately CI did not test this fix properly - the "green tick" ✔️ was a false one as no tests ran. I'll have to look at this more closely now

@TIHan
Copy link
Contributor

TIHan commented Jun 16, 2020

There is one test that is failing at the moment.

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

Approved - pending test passes.

@dsyme
Copy link
Contributor Author

dsyme commented Jun 16, 2020

There is one test that is failing at the moment.

RIght, that's because the fix didn't work :-)))

I've pushed a different fix, let's see if this one is green. The same code is removed, but the problem that code was addressing is now dealt with differently.

Notes:

  • The "CodegenWitness..." code is a late rediscovery of the solutions to constraints

  • To do this for a call to a method M<T1, ... TN> at type arguments M<ty1...tyN> we freshen type parameters T1...TN and assert equalities T1 = ty1 ... TN = tyN, propagating constraints as we asset each equality, and see what constraint solutions were found

  • The process of asserting these constraints one by one causes problems where half way through there appear to be unmet constraints. Previously I'd worked around this incorrectly by allowing these unmet constraints to be added to the type parameters during codegen. This just isn't right.

  • Instead, we should assert the equalities simultaneously then propagate constraints.

Basically when you have a complicated tangle of constraints involving a bunch of type parameters there needs to be simultaneous processing here

@KevinRansom KevinRansom merged commit 8044b5f into dotnet:master Jun 16, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* alternative fix for 9449

* add test case

* fix 9449 properly by assert type equations simultaneously
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