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

Make InvalidGenerationSourceError an Exception #687

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

natebosch
Copy link
Member

See dart-lang/build#3585

Rename to InvalidGenerationSource and change to a subtype of
Exception. This class is not thrown to indicate a programming error in
the code that is running, it is thrown to indicate an error in the code
under analysis. It was originally implemented without a super type and
the name ending in "Error" was not specifically discussed, but it was
intended to convey an "error in build input", not an error in the
builder implementation (where it is thrown). Later a lint required it to
be a subtype of either Error or Exception, and the Error supertype
was chosen without discussion because of the name, even though it's not
thrown as an Error in the sense where that distinction matters.

Add a type alias for the exception with the old name. This can be
deprecated whenever we have the bandwidth to handle the cleanup, but it
will not be deprecated immediately.

Change from extends Error to implements Exception. This is
technically breaking, but it is unlikely to make a significant impact in
real world usage scenarios. It may impact how the error surfaces to end
users in some places.

See dart-lang/build#3585

Rename to `InvalidGenerationSource` and change to a subtype of
`Exception`. This class is not thrown to indicate a programming error in
the code that is running, it is thrown to indicate an error in the code
under analysis. It was originally implemented without a super type and
the name ending in "Error" was not specifically discussed, but it was
intended to convey an "error in build input", not an error in the
builder implementation (where it is thrown). Later a lint required it to
be a subtype of either `Error` or `Exception`, and the `Error` supertype
was chosen without discussion because of the name, even though it's not
thrown as an `Error` in the sense where that distinction matters.

Add a type alias for the exception with the old name. This can be
deprecated whenever we have the bandwidth to handle the cleanup, but it
will not be deprecated immediately.

Change from `extends Error` to `implements Exception`. This is
technically breaking, but it is unlikely to make a significant impact in
real world usage scenarios. It may impact how the error surfaces to end
users in some places.
@natebosch
Copy link
Member Author

@kevmoo - do you have concerns about the potential breakage cause by changing an extends Error to implements Exception?

@natebosch
Copy link
Member Author

This change did have a successful global presubmit.

@kevmoo
Copy link
Member

kevmoo commented Oct 3, 2023

@kevmoo - do you have concerns about the potential breakage cause by changing an extends Error to implements Exception?

Not really? What are the failure modes here?

@natebosch
Copy link
Member Author

What are the failure modes here?

The only thing I can think of is something like a builder may have an outer try/catch on Exception or zone error handler which they expect to not catch InvalidGenerationSource (because that already has a user friendly toString that they want to expose).

The context is that we are going to start showing stack traces (even without -v) for caught Error, because we expect most of those to be bugs in the builder implementation, and the stack trace can be helpful in bug reports.

Some builders use InvalidGenerationSourceError (correctly) to report issues with the input code, and we don't want those to show a stack trace by default.

@natebosch
Copy link
Member Author

an outer try/catch on Exception or zone error handler which they expect to not catch InvalidGenerationSource

Even in these cases, the worst I expect is potentially changed error reporting back to the user. It's obviously not likely to change any happy path behavior.

@natebosch natebosch merged commit c8d8873 into master Oct 5, 2023
12 checks passed
@natebosch natebosch deleted the invalid-generation-source branch October 5, 2023 19:15
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