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

More reductions in error handling diversity #67770

Merged
merged 2 commits into from
Jan 8, 2020

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jan 1, 2020

In this follow up to #67744, we:

  • Remove all fatal / error / warning macros in syntax except for struct_span_err, which is moved to rustc_errors.

  • Lintify some hard-coded warnings which used warning macros.

  • Defatalize some errors.

In general, the goal here is to make it painful to use fatal or unstructured errors and so we hopefully won't see many of these creep in.

Fixes #67933.

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 1, 2020
@Centril
Copy link
Contributor Author

Centril commented Jan 1, 2020

r? @Mark-Simulacrum

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Some of my comments I think get obsoleted by future commits, but along with them, two higher-level comments:

  • no real point to moving span_err and friends to struct_span_err(...).emit()
  • it is not clear that removing the macros, rather than (e.g.) moving to rustc_errors is helpful

I'm also not super happy with moving things to lints in this PR; feels like maybe there's some discussion to be had, and it's not clear that it brings us benefits from the PR goals perspective. Maybe we could split those commits out? I don't know if we need (e.g.) sign off or so. Seems like probably no, and indeed moving to lints is good.

Would also be good to do some rebase magic and cleanup commits (a few are misspelled or essential duplicates of each other, e.g., I would roll in all the "unused" macro removal into the ~last commit which removes the syntax::diagnostics::macros module.

src/test/ui/lint/lint-uppercase-variables.stderr Outdated Show resolved Hide resolved
src/librustc_metadata/locator.rs Show resolved Hide resolved
src/librustc_typeck/structured_errors.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Show resolved Hide resolved
@petrochenkov petrochenkov self-assigned this Jan 1, 2020
@bors

This comment has been minimized.

@Centril Centril force-pushed the reduce-diversity-2 branch 5 times, most recently from c34ca3b to be8f29e Compare January 1, 2020 21:59
@Centril
Copy link
Contributor Author

Centril commented Jan 1, 2020

(For the benefit of everyone reading, here's a summary of notes I left on Discord...)

  • no real point to moving span_err and friends to struct_span_err(...).emit()

The point is to encourage adding .span_suggestion(...), .span_label(...) and whatnot by not immediately emitting the error ("the right thing should be more convenient"). Eventually we can maybe rename to error!(...).emit(); once diversity is further reduced.

it is not clear that removing the macros, rather than (e.g.) moving to rustc_errors is helpful

I think we should make it harder to emit unstructured / fatal errors as well as hard-coded warnings, and those macros are in the way of that. Also, the diagnostics builder API is good enough that help!(...) et. al feels unnecessary.

I'm also not super happy with moving things to lints in this PR; feels like maybe there's some discussion to be had, and it's not clear that it brings us benefits from the PR goals perspective. Maybe we could split those commits out? I don't know if we need (e.g.) sign off or so. Seems like probably no, and indeed moving to lints is good.

So lintifying these enabled nixing some of the macros. Beyond that, hard-coded warnings are not something we want outside of things like the NLL transition (which are super rare and don't deserve sugar).

Would also be good to do some rebase magic and cleanup commits (a few are misspelled or essential duplicates of each other, e.g., I would roll in all the "unused" macro removal into the ~last commit which removes the syntax::diagnostics::macros module.

Squashed all into one commit. :)

@Mark-Simulacrum
Copy link
Member

Changes look fine to me overall; probably makes sense to wait for @petrochenkov here though.

@petrochenkov
Copy link
Contributor

Eventually we can maybe rename to error!(...).emit();

+1, if the errors are spanned and structural by default, then it doesn't make sense to write struct_span.

@petrochenkov
Copy link
Contributor

We also probably need a variation of struct_span_err without an error code though, errors with codes are not an overwhelming majority right now, unlike e.g. spanned errors.
(But that kind of depends on the decision about error codes in general.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2020
@Centril
Copy link
Contributor Author

Centril commented Jan 2, 2020

The variant without a code is just self.struct_span_err(...) (not a macro) though? (It works well enough in the parser... At least I've never wanted for more.)

@petrochenkov
Copy link
Contributor

@Centril
You have to write &format!(...) with self.struct_span_err, which is kind of annoying.

@Centril
Copy link
Contributor Author

Centril commented Jan 2, 2020

Review comments should be addressed now (except for the new variation on the macro, which I think could be done some other time).

@rust-highfive

This comment has been minimized.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 8, 2020
- remove syntax::{help!, span_help!, span_note!}
- remove unused syntax::{struct_span_fatal, struct_span_err_or_warn!, span_err_or_warn!}
- lintify check_for_bindings_named_same_as_variants + conflicting_repr_hints
- inline syntax::{struct_span_warn!, diagnostic_used!}
- stringify_error_code! -> error_code! & use it more.
- find_plugin_registrar: de-fatalize an error
- de-fatalize metadata errors
- move type_error_struct! to rustc_typeck
- struct_span_err! -> rustc_errors
@Centril
Copy link
Contributor Author

Centril commented Jan 8, 2020

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jan 8, 2020

📌 Commit 20ebb80 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 8, 2020
@bors
Copy link
Contributor

bors commented Jan 8, 2020

⌛ Testing commit 20ebb80 with merge ed6468d...

bors added a commit that referenced this pull request Jan 8, 2020
More reductions in error handling diversity

In this follow up to #67744, we:

- Remove all fatal / error / warning macros in `syntax` except for `struct_span_err`, which is moved to `rustc_errors`.

- Lintify some hard-coded warnings which used warning macros.

- Defatalize some errors.

In general, the goal here is to make it painful to use fatal or unstructured errors and so we hopefully won't see many of these creep in.

Fixes #67933.
@bors
Copy link
Contributor

bors commented Jan 8, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing ed6468d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 8, 2020
@bors bors merged commit 20ebb80 into rust-lang:master Jan 8, 2020
@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril Centril deleted the reduce-diversity-2 branch January 8, 2020 18:36
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
Extract `rustc_ast_passes`, move gating, & refactor linting

Based on rust-lang#67770.

This PR extracts a crate `rustc_ast_passes`:

- `ast_validation.rs`, which is contributed by `rustc_passes` (now only has HIR based passes) -- the goal here is to improve recompilation of the parser,
- `feature_gate.rs`, which is contributed by `syntax` and performs post-expansion-gating & final erroring for pre-expansion gating,
- `show_span`, which is contributed by `syntax`.

To facilitate this, we first have to also:

- Move `{leveled_}feature_err{_err}` from `syntax::feature_gate::check` into `rustc_session::parse`.
- Move `get_features` into `rustc_parse::config`, the only place it is used.
- Move some some lint datatypes and traits, e.g. `LintBuffer`, `BufferedEarlyLint`, `BuiltinLintDiagnostics`, `LintPass`, and `LintArray` into `rustc_session::lint`.
- Move all the hard-wired lint `static`s into `rustc_session::lint::builtin`.
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
Extract `rustc_ast_passes`, move gating, & refactor linting

Based on rust-lang#67770.

This PR extracts a crate `rustc_ast_passes`:

- `ast_validation.rs`, which is contributed by `rustc_passes` (now only has HIR based passes) -- the goal here is to improve recompilation of the parser,
- `feature_gate.rs`, which is contributed by `syntax` and performs post-expansion-gating & final erroring for pre-expansion gating,
- `show_span`, which is contributed by `syntax`.

To facilitate this, we first have to also:

- Move `{leveled_}feature_err{_err}` from `syntax::feature_gate::check` into `rustc_session::parse`.
- Move `get_features` into `rustc_parse::config`, the only place it is used.
- Move some some lint datatypes and traits, e.g. `LintBuffer`, `BufferedEarlyLint`, `BuiltinLintDiagnostics`, `LintPass`, and `LintArray` into `rustc_session::lint`.
- Move all the hard-wired lint `static`s into `rustc_session::lint::builtin`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[deny(warnings) isn't working on E0170
6 participants