Skip to content

Fix #5768, Corrected wrong formatted print of a trait member constraint. - #5948

Closed
realvictorprm wants to merge 5 commits into
dotnet:masterfrom
realvictorprm:srtp_constraint_printing_fix
Closed

Fix #5768, Corrected wrong formatted print of a trait member constraint.#5948
realvictorprm wants to merge 5 commits into
dotnet:masterfrom
realvictorprm:srtp_constraint_printing_fix

Conversation

@realvictorprm

Copy link
Copy Markdown
Contributor

The bug was not that the constraint is ignored. It was that the compiler printed the suggested trait constraint wrongly.

As example, the signature a * b -> (c -> d) isn't the same as a * b -> c -> d because the return type differs (in case one it's (c -> d), in case two it's d). The compiler however didn't respect that behaviour and prints constraints of case 1 always like case 2 which is wrong. The fix was to enforce that trait constraint return type is always wrapped in brackets if the return type is a function.

@dsyme please have a look whether this assumption is fine? It's a short thing only about formatting.

Signed-off-by: realvictorprm mueller.vpr@gmail.com

Corrected wrong formatted print of a trait member constraint
Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
@realvictorprm

Copy link
Copy Markdown
ContributorAuthor

@cartermp I won't the neat command "@dotnetbot please test this" back 😭 Closing isn't nice.

@realvictorprm

Copy link
Copy Markdown
ContributorAuthor

CI passes. This is ready.

@saul

saul commented Nov 23, 2018

Copy link
Copy Markdown
Contributor

Can we add the example in #5768 as a test case to make sure we don't regress?

@realvictorprm

Copy link
Copy Markdown
ContributorAuthor

I'll have a look how I can add it.

@dsymedsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Needs test, otherwise approved

Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
@realvictorprm

Copy link
Copy Markdown
ContributorAuthor

Regression test added.

Comment threadtests/fsharp/tests.fs Outdated

@dsymedsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The fix is fine. The testing could be simpler I believe

Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
@realvictorprm

Copy link
Copy Markdown
ContributorAuthor

@dsyme I've changed it, please have a short look over it.

@realvictorprm

Copy link
Copy Markdown
ContributorAuthor

CI passes, can this be merged?

@dsyme

dsyme commented Jan 9, 2019

Copy link
Copy Markdown
Contributor

approved, please resolve conflict and we can merge

@realvictorprm

Copy link
Copy Markdown
ContributorAuthor

done

@dsymedsyme closed this Feb 22, 2019
@dsymedsyme reopened this Feb 22, 2019
@KevinRansomKevinRansom reopened this Mar 9, 2019
@cartermp

cartermp commented Jun 27, 2020

Copy link
Copy Markdown
Contributor

Ergh, looks like there were some build issues preventing this from getting merged and the codebase has diverged since. Sorry about that. @realvictorprm are you still interested in pursuing this? In theory it should be easy to adjust the tests and just copy the contents of this branch's neg111.fs and neg111.bsl into new files.

@abelbraaksma

abelbraaksma commented Jul 21, 2020

Copy link
Copy Markdown
Contributor

@cartermp, in an attempt not to lose this work, I've resurrected it here: #9728 (branched off this one to keep history intact). It does exactly what you suggest with the conflicting files.

@cartermp

Copy link
Copy Markdown
Contributor

Thanks! Will close in favor of #9728

Sign up for freeto 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.

7 participants

@realvictorprm@saul@dsyme@cartermp@abelbraaksma@ctrlaltvikas@KevinRansom