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

Resolver should only throw for severe errors #3828

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dickermoshe
Copy link

Closes #3827

While using a code generator package I ran into an issue where I was being told that a file had a syntax error.
There was no error, analyzer reported no issues.

After further debugging, it turned out to be a Doc directive 'hello' is unknown. error.
Because this error is not severe, it should be ignored.

This PR makes build_resolver only throw on severe syntax errors


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Copy link

github-actions bot commented Feb 6, 2025

Package publishing

Package Version Status Publish tag (post-merge)
package:build 2.4.2 already published at pub.dev
package:build_config 1.1.2 already published at pub.dev
package:build_daemon 4.0.3 already published at pub.dev
package:build_modules 5.0.11 already published at pub.dev
package:build_resolvers 2.4.4-wip WIP (no publish necessary)
package:build_runner 2.4.15-wip WIP (no publish necessary)
package:build_runner_core 8.0.1-dev ready to publish build_runner_core-v8.0.1-dev
package:build_test 2.2.4-wip WIP (no publish necessary)
package:build_web_compilers 4.1.1 already published at pub.dev
package:scratch_space 1.0.3-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

Copy link

github-actions bot commented Feb 6, 2025

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Copy link
Contributor

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

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

Thanks! I ran into the exact same issue with @code a few months ago, I had to go digging, turns out I "fixed" it for only one codepath, the analyzer's "sort directives":

https://dart-review.googlesource.com/c/sdk/+/391740/3/pkg/analysis_server/lib/src/g3/utilities.dart

The parsed results "errors" do indeed include warnings and infos, and it's the correct thing to ignore them here.

Could you please add a unit test? In build_resolvers/test/resolver_test.dart it should work to copy the are reported test, add the @code case instead of the syntax error, and check that it doesn't throw.

@dickermoshe
Copy link
Author

I've added the relevant tests
Thanks @davidmorgan & @HosseinYousefi for this expedited review.

Copy link
Contributor

@davidmorgan davidmorgan 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, thanks! Let's wait for @jakemac53 to take a look also, this is my first external PR review for build_runner.

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.

comment with {@code} causes build_runner to crash
2 participants