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.

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

Could build_runner provide a stacktrace when builders throw an exception? #3585

Closed
rrousselGit opened this issue Sep 28, 2023 · 5 comments · Fixed by #3586
Closed

Could build_runner provide a stacktrace when builders throw an exception? #3585

rrousselGit opened this issue Sep 28, 2023 · 5 comments · Fixed by #3586

Comments

@rrousselGit
Copy link

Hello!

One inconvenience I've had for a while is, if a code-generator fails, no stacktrace is available.
That makes debugging quite difficult.
For example, I've received the following issue: rrousselGit/riverpod#2798:

[...]
[SEVERE] riverpod_generator on lib/app/state/animation.viewmodel.dart (cached):

Stack Overflow
[SEVERE] riverpod_generator on lib/app/state/places_view.viewmodel.dart (cached):

Stack Overflow
[SEVERE] riverpod_generator on lib/app/state/selected.viewmodel.dart (cached):

Stack Overflow
[SEVERE] Failed after 178ms

The user provided their console logs. But the error by itself is unhelpful.
@jakemac53
Copy link
Contributor

jakemac53 commented Sep 28, 2023 •
edited
Loading

If you pass -v it should print the stack traces (and this should also work for a no-op re-run, it will re-log previous errors).

@rrousselGit
Copy link
Author

The main issue is, I have no control over what issue authors write.

Take the issue linked for example: Besides asking for more information, I can't do much.

If stacktraces were included by default, when users report issues, I'd be much more likely to be able to fix the issue in a short time-frame.

@natebosch
Copy link
Member

@jakemac53 - WDYT about printing the stack trace by default for subtypes of Error, but not Exception?

I think it's intended behavior that a builder author can throw an Exception with a nice .toString() and have it not print the stack trace without -v. As long as builder authors are following best practice and not using Error for this, then we can print stack traces for Error subtypes and expect most of them to be bugs in the implementation and not intentional build failures.

@natebosch natebosch mentioned this issue Sep 28, 2023
Merged
natebosch added a commit that referenced this issue Sep 28, 2023
@natebosch
Always log stack traces for Error objects
03b21a4 
Closes #3585

The default logging omits stack traces by default and prints them only
in verbose mode. This is intended to allow builder implementations to
cease execution and log a user friendly message by throwing a single
`Exception` with a custom `toString()`. When a builder or utility
library has an implementation bug leading to an `Error` the stack trace
is much more likely to be useful information for debugging, and worth
including by default.
@jakemac53
Copy link
Contributor

SGTM

@rrousselGit
Copy link
Author

In that case we'd want to make InvalidSourceGenerationError to be an exception instead https://pub.dev/documentation/source_gen/latest/source_gen/InvalidGenerationSourceError-class.html

I think many code generators rely on it

natebosch added a commit to dart-lang/source_gen that referenced this issue Oct 3, 2023
@natebosch
Make InvalidGenerationSourceError an Exception
b2ac195 
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 natebosch mentioned this issue Oct 3, 2023
Merged
natebosch added a commit to dart-lang/source_gen that referenced this issue Oct 5, 2023
@natebosch
Make InvalidGenerationSourceError an Exception (#687)
c8d8873 
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 added a commit that referenced this issue Dec 16, 2023
@natebosch
Always log stack traces for Error objects (#3586)
ca7f881 
Closes #3585

The default logging omits stack traces by default and prints them only
in verbose mode. This is intended to allow builder implementations to
cease execution and log a user friendly message by throwing a single
`Exception` with a custom `toString()`. When a builder or utility
library has an implementation bug leading to an `Error` the stack trace
is much more likely to be useful information for debugging, and worth
including by default.
mosuem pushed a commit that referenced this issue Dec 10, 2024
@natebosch
Make InvalidGenerationSourceError an Exception (dart-lang/source_gen#687
639af48 
)

See #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.
mosuem pushed a commit that referenced this issue Dec 10, 2024
@natebosch
Make InvalidGenerationSourceError an Exception (dart-lang/source_gen#687
6651363 
)

See #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Always log stack traces for Error objects dart-lang/build
3 participants
@jakemac53 @natebosch @rrousselGit