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

Fix Issue 222708 - switch statement with an undefined symbol results … #13616

Closed
wants to merge 2 commits into from

Conversation

maxhaton
Copy link
Member

@maxhaton maxhaton commented Feb 7, 2022

…in many errors

The fix proposed bails out of the semantic analysis early.
This reduces the false errors to nil at the expense of delaying some error messages to the next compile e.g. duplicate cases

…in many errors

The fix proposed bails out of the semantic analysis early.
This reduces the false errors to nil at the expense of delaying some error messages to the next compile e.g. duplicate cases
@maxhaton
Copy link
Member Author

maxhaton commented Feb 7, 2022

@schveiguy

Copy link
Contributor

@MoonlightSentinel MoonlightSentinel left a comment

Choose a reason for hiding this comment

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

The new error message isn't really helpful. It would be better to bail silently IMHO.

Alternative proposal: #13617

@maxhaton
Copy link
Member Author

maxhaton commented Feb 7, 2022

The new error message isn't really helpful. It would be better to bail silently IMHO.

Alternative proposal: #13617

It costs nothing and it explains the issue more thoroughly to those new to the language. Ideally they would come the other way round with some indentation but I can't be bothered to do that.

@@ -24,6 +24,7 @@ void test1()
TEST_OUTPUT:
---
fail_compilation/test_switch_error.d(105): Error: undefined identifier `doesNotExist`
fail_compilation/test_switch_error.d(105): Error: The condition expression in this switch statement failed to compile
Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. this error message is redundant to the already existing message w.r.t. the undefined identifier. I read it as "There's another error here" even though it's an addition to the existing error message. Might be improved by using errorSupplemental instead of error.

Copy link
Member Author

@maxhaton maxhaton Feb 7, 2022

Choose a reason for hiding this comment

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

It should be error supplemental but the other way round but that would mean saving the error from the expression semantic which I can't be bothered to do tonight at least. An API like a gag but for basically grouping errors might be a nice addition.

In a sea of errors this isn't useless either. It looks redundant, but almost all errors are redundant in a certain sense, we aren't the target demographic for error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, we might not be the best candidates to evaluate error messages.

Offering all required informations is usually a good idea but also requires delicacy - too much noise is detrimental as well. This snippet is a good example because the error is completely unrelated to the switch statement,

@RazvanN7
Copy link
Contributor

@maxhaton it seems that @MoonlightSentinel has a better fix for this, I suggest we go with #13617

@maxhaton
Copy link
Member Author

@maxhaton it seems that @MoonlightSentinel has a better fix for this, I suggest we go with #13617

Sure, I want to try to get this style of error message but in the opposite order as it appears here. Happy to close

@maxhaton maxhaton closed this Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants