Skip to content

Give better error message when IF Statement has context requirements #3150

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
May 31, 2017
Merged

Give better error message when IF Statement has context requirements #3150

merged 8 commits into from
May 31, 2017

Conversation

forki
Copy link
Contributor

@forki forki commented May 29, 2017 •
edited
Loading

fixes #3146

@forki forki mentioned this pull request May 29, 2017
Closed
@forki forki changed the title Give better error message when IF Statement has context requirements [WIP] Give better error message when IF Statement has context requirements May 29, 2017
saul
saul reviewed May 29, 2017
View reviewed changes
Copy link
Contributor

@saul saul left a comment

Choose a reason for hiding this comment

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

Looks good - could do with a test case?

src/fsharp/FSComp.txt Outdated
@@ -17,6 +17,7 @@ undefinedNameTypeParameter,"The type parameter %s is not defined."
undefinedNamePatternDiscriminator,"The pattern discriminator '%s' is not defined."
replaceWithSuggestion,"Replace with '%s'"
missingElseBranch,"The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, and not a statement, add an 'else' branch which returns a value of the same type."
ifExpression,"The 'if' expression needs to return type '%s' in order to satisfy context type requirements. It currently returns type '%s'."
Copy link
Contributor

Choose a reason for hiding this comment

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

'Returns' may be a bit of a misnomer here - the expression itself has a type. Maybe just: "The 'if' expressions needs to have type '%s' in order to satisfy context type requirements. It currently has type '%s'."

Also out of interest what is a 'context type requirement'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm if you have to ask then it's probably not good.
I wanted to tell that the type requirement comes from the context of the expression - not from the expression itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saul @isaacabraham how could we express this better?

@forki
Copy link
Contributor Author

forki commented May 30, 2017

Agent went offline during the build - sigh

@forki forki closed this May 30, 2017
@forki forki reopened this May 30, 2017
@forki forki changed the title [WIP] Give better error message when IF Statement has context requirements Give better error message when IF Statement has context requirements May 30, 2017
saul
saul suggested changes May 30, 2017
View reviewed changes
src/fsharp/FSComp.txt Outdated
@@ -17,6 +17,7 @@ undefinedNameTypeParameter,"The type parameter %s is not defined."
undefinedNamePatternDiscriminator,"The pattern discriminator '%s' is not defined."
replaceWithSuggestion,"Replace with '%s'"
missingElseBranch,"The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, and not a statement, add an 'else' branch which returns a value of the same type."
ifExpression,"The 'if' expression needs to return type '%s' in order to satisfy context type requirements. It currently has type '%s'."
Copy link
Contributor

Choose a reason for hiding this comment

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

"needs to return type" -> "must be of type"

Copy link
Contributor

Choose a reason for hiding this comment

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

you can lose the 'in order' as well.

src/fsharp/FSComp.txt
@@ -17,6 +17,7 @@ undefinedNameTypeParameter,"The type parameter %s is not defined."
undefinedNamePatternDiscriminator,"The pattern discriminator '%s' is not defined."
replaceWithSuggestion,"Replace with '%s'"
missingElseBranch,"The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, and not a statement, add an 'else' branch which returns a value of the same type."
Copy link
Contributor

Choose a reason for hiding this comment

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

"returns a value of the same type" -> "has a value of the same type"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with the other PR

src/fsharp/FSComp.txt Outdated
@@ -17,6 +17,7 @@ undefinedNameTypeParameter,"The type parameter %s is not defined."
undefinedNamePatternDiscriminator,"The pattern discriminator '%s' is not defined."
replaceWithSuggestion,"Replace with '%s'"
missingElseBranch,"The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, and not a statement, add an 'else' branch which returns a value of the same type."
ifExpression,"The 'if' expression needs to return type '%s' in order to satisfy context type requirements. It currently has type '%s'."
elseBranchHasWrongType,"All branches of an 'if' expression must return the same type. This expression was expected to have type '%s' but here has type '%s'."
Copy link
Contributor

Choose a reason for hiding this comment

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

"must return the same type" -> "must be of the same type"

KevinRansom
KevinRansom approved these changes May 30, 2017
View reviewed changes
Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Approved with a minor tweak to the message text.

src/fsharp/FSComp.txt Outdated
@@ -17,6 +17,7 @@ undefinedNameTypeParameter,"The type parameter %s is not defined."
undefinedNamePatternDiscriminator,"The pattern discriminator '%s' is not defined."
replaceWithSuggestion,"Replace with '%s'"
missingElseBranch,"The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, and not a statement, add an 'else' branch which returns a value of the same type."
ifExpression,"The 'if' expression needs to return type '%s' in order to satisfy context type requirements. It currently has type '%s'."
Copy link
Contributor

Choose a reason for hiding this comment

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

you can lose the 'in order' as well.

@forki
Copy link
Contributor Author

forki commented May 31, 2017

fixed wording

@KevinRansom KevinRansom merged commit 24ea91b into dotnet:master May 31, 2017
@KevinRansom
Copy link
Contributor

@forki thanks for this.

Kevin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saul saul saul requested changes

@KevinRansom KevinRansom KevinRansom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent error message for incompatible return type in nested if / else
4 participants
@forki @KevinRansom @saul @msftclas