Skip to content

Restored change from error to warning from CL 85959 #255

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

Closed
wants to merge 1 commit into from

Conversation

eernstg
Copy link
Member

@eernstg eernstg commented Mar 6, 2019

The change from 'compile-time error' to 'static warning' that we performed in CL 85959 was accidentally lost when 'invalid_returns.md' was integrated into the language specification. This CL just changes those few words back to 'static warning' (and removes a couple of spurious {}).

@eernstg eernstg requested a review from lrhn March 6, 2019 11:56
@lrhn
Copy link
Member

lrhn commented Mar 6, 2019

I still think this should be an error.
@leafpetersen

@eernstg
Copy link
Member Author

eernstg commented Mar 6, 2019

We could do that:

Given that tools have reported an error for this situation for a while (we currently get expectation 'StaticWarning' and actual outcome 'CompileTimeError' for, e.g., analyzer-linux-release language_2/void/return_future_future_or_void_async_error1_test/none), we are in a good position to just keep the specification unchanged (so it remains an error, now per decision rather than by accident), that is: we'd just delete this pull request.

We might want to consider all the other warnings an extra time (maybe some of them can also be errors), but in that case we'd need to change the specification, so that would be done in a separate PR.

@leafpetersen
Copy link
Member

(we currently get expectation 'StaticWarning' and actual outcome 'CompileTimeError' for, e.g., analyzer-linux-release language_2/void/return_future_future_or_void_async_error1_test/none)

This seems to have been an (accidental?) regression between 2.0 (where the analyzer reports a warning) and 2.1 (where it is an error). Maybe reasonable to move forward with it since it's been this way for a while? Not great that this slipped through though.

@eernstg
Copy link
Member Author

eernstg commented Jul 5, 2019

Abandoned. At this point we're more likely to change even more warnings to errors, rather than changing these two errors to warnings.

@eernstg eernstg closed this Jul 5, 2019
@eernstg eernstg deleted the fix_return_warning_mar19 branch July 5, 2019 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants