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

Add ErrorGuaranteed to ast::ExprKind::Err #120586

Merged
merged 3 commits into from
Feb 26, 2024
Merged

Conversation

ShE3py
Copy link
Contributor

@ShE3py ShE3py commented Feb 2, 2024

See #119967 for context

      \
       \
          _~^~^~_
      \) /  o o  \ (/
        '_   -   _'
        / '-----' \

r? fmease

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

ExprKind::Err(guar) => hir::ExprKind::Err(*guar),

ExprKind::Dummy => {
span_bug!(e.span, "lowered ExprKind::Dummy")
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for working on this!

I have a couple of minor suggestions but there are still some parser changes I need to review. Note that I probably won't approve this until CI works again (oh, it runs again!). The tree is closed anyway.

You also need to to update src/tools/rustfmt (ah, you just pushed them haha). I see that you've already updated Clippy, amazing!

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
compiler/rustc_builtin_macros/src/concat_bytes.rs Outdated Show resolved Hide resolved
compiler/rustc_builtin_macros/src/concat_bytes.rs Outdated Show resolved Hide resolved
compiler/rustc_builtin_macros/src/deriving/default.rs Outdated Show resolved Hide resolved
return false;
}
}
TokenTree::Delimited(.., del) => check_lhs_no_empty_seq(sess, &del.tts)?,
Copy link
Member

Choose a reason for hiding this comment

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

🎉

compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/expr.rs Show resolved Hide resolved
compiler/rustc_parse/src/parser/stmt.rs Outdated Show resolved Hide resolved
@fmease fmease 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 Feb 2, 2024
@ShE3py
Copy link
Contributor Author

ShE3py commented Feb 3, 2024

Refactored some code, also took the liberty to remove the ast:: invasion in the builtin macros I modified.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2024
@@ -143,7 +143,7 @@ pub fn eq_expr(l: &Expr, r: &Expr) -> bool {
match (&l.kind, &r.kind) {
(Paren(l), _) => eq_expr(l, r),
(_, Paren(r)) => eq_expr(l, r),
(Err, Err) => true,
(Err(_), Err(_)) => true,
Copy link
Member

Choose a reason for hiding this comment

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

(Dummy, Dummy) is missing from this match statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To mimic the old behavior (Err(_), Dummy) and (Dummy, Err(_)) may also be needed, and I wonder if (Dummy, _) | (_, Dummy) => true makes sense.

But the only two occurences of ExprKind::Dummy are in

  • <Expr as DummyAstNode>::dummy() which isn't actually called (I can replace its body with panic!() and both ./x test --stage 1 tests/ui/ and ./x check --stage 2 passes successfully)
    impl DummyAstNode for Expr {
    fn dummy() -> Self {
    Expr {
    id: DUMMY_NODE_ID,
    kind: ExprKind::Err,
    span: Default::default(),
    attrs: Default::default(),
    tokens: Default::default(),
    }
    }
    }
  • Parser::maybe_recover_trailing_expr() for avoiding creating a P<Expr>
    // Parse `?`, `.f`, `(arg0, arg1, ...)` or `[expr]` until they've all been eaten.
    if let Ok(expr) = snapshot
    .parse_expr_dot_or_call_with(
    self.mk_expr_err(pat_span), // equivalent to transforming the parsed pattern into an `Expr`
    pat_span,
    AttrVec::new(),
    )

So ast_utils.rs's eq_expr cannot actually have an ExprKind::Dummy, so this case can't be tested in the testsuite. I was tempted to put a panic!() to hold the choice until later if someone uses Dummy and it reaches Clippy, but if it's not tested I'd rather have a wrong diagnostic than a panic in prod.

Copy link
Member

@flip1995 flip1995 Feb 15, 2024

Choose a reason for hiding this comment

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

If this can really not occur here, then I suggest to add a debug_assertion or an unreachable! in this function. I'd like this to be an exhaustive match.

@fmease
Copy link
Member

fmease commented Feb 8, 2024

Just to give an update: I'm at 35 / 39 files viewed with pending review comments. Finishing my review later.

@nnethercote
Copy link
Contributor

nnethercote commented Feb 14, 2024

Oh dear, I started working on this today, not realizing that someone else was working on it. I also did TyKind::Err (#121072) and LitKind::Err (mostly done, but no PR yet).

I didn't get all the way to fixing ErrKind::Err, but I think it's possible to do this without introducing ErrKind::Dummy, which would reduce the size of the diff.

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Just posting my pending review comments. I still need to look at some of these files.

compiler/rustc_parse/src/validate_attr.rs Outdated Show resolved Hide resolved
compiler/rustc_builtin_macros/src/source_util.rs Outdated Show resolved Hide resolved
@@ -1065,7 +1071,7 @@ pub struct ExtCtxt<'a> {
pub sess: &'a Session,
pub ecfg: expand::ExpansionConfig<'a>,
pub num_standard_library_imports: usize,
pub reduced_recursion_limit: Option<Limit>,
pub reduced_recursion_limit: Option<(Limit, ErrorGuaranteed)>,
Copy link
Member

@fmease fmease Feb 8, 2024

Choose a reason for hiding this comment

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

Wondering if this change is strictly required or if this can stay an Option<Limit> unless it forces us to introduce delayed_bug somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if !recursion_limit.value_within_limit(self.cx.current_expansion.depth) {
if self.cx.reduced_recursion_limit.is_none() {
self.error_recursion_limit_reached();
}
// Reduce the recursion limit by half each time it triggers.
self.cx.reduced_recursion_limit = Some(recursion_limit / 2);
return ExpandResult::Ready(invoc.fragment_kind.dummy(invoc.span()));
}

The error is only emitted once (MacroExpander::error_recursion_limit_reached) and AstFragmentKind::dummy requires a guar.

It's possible to use DiagCtxt::has_errors().unwrap(), but we lose the guarantee that if the recursion limit was reduced, then an error was emitted.

compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
@@ -388,7 +388,7 @@ impl<'a> Parser<'a> {
// Parse `?`, `.f`, `(arg0, arg1, ...)` or `[expr]` until they've all been eaten.
if let Ok(expr) = snapshot
.parse_expr_dot_or_call_with(
self.mk_expr_err(pat_span), // equivalent to transforming the parsed pattern into an `Expr`
self.mk_expr_dummy(pat_span), // equivalent to transforming the parsed pattern into an `Expr`
Copy link
Member

Choose a reason for hiding this comment

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

Oh, it's unfortunate that we have to use ExprKind::Dummy here, I wish we wouldn't introduce more uses of it but I don't see a better option here either.

@fmease
Copy link
Member

fmease commented Feb 14, 2024

[…] I think it's possible to do this without introducing ErrKind::Dummy, which would reduce the size of the diff.

@nnethercote How would you go about updating trait DummyAstNode and everything that uses it without ExprKind::Dummy?

@bors
Copy link
Contributor

bors commented Feb 14, 2024

☔ The latest upstream changes (presumably #121078) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Copy link
Contributor

@nnethercote How would you go about updating trait DummyAstNode and everything that uses it without ExprKind::Dummy?

Use ExprKind::Tup(ThinVec::new()) wherever possible, and only use ExprKind::Err if absolutely necessary. DummyAstNode for PatKind and (soon) TyKind use a non-error value. And DummyResult::raw_expr is another example that uses ExprKind::Tup(ThinVec::new()) where possible.

Because I didn't fully convert ExprKind::Err myself I'm not 100% sure, but it really seems like "use Tup(ThinVec::new()) wherever possible and Err(guar) everywhere else" should be enough.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 15, 2024
…ease

Add `ErrorGuaranteed` to `ast::LitKind::Err`, `token::LitKind::Err`.

Similar to recent work doing the same for `ExprKind::Err` (rust-lang#120586) and `TyKind::Err` (rust-lang#121109).

r? `@oli-obk`
@bors
Copy link
Contributor

bors commented Feb 26, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 26, 2024
@fmease
Copy link
Member

fmease commented Feb 26, 2024

FAILED: lib/liblldMachO.a
C:\a\rust\rust\mingw64\bin\ranlib.exe: could not create temporary file whilst writing archive: no more archived files

@bors retry (spurious?)

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
Add `ErrorGuaranteed` to `ast::ExprKind::Err`

See rust-lang#119967 for context
```
      \
       \
          _~^~^~_
      \) /  o o  \ (/
        '_   -   _'
        / '-----' \
```

r? fmease
@bors
Copy link
Contributor

bors commented Feb 26, 2024

⌛ Testing commit 34eae07 with merge 99eba8f...

@bors
Copy link
Contributor

bors commented Feb 26, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 26, 2024
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#1 [internal] booting buildkit
#1 pulling image moby/buildkit:buildx-stable-1
#1 pulling image moby/buildkit:buildx-stable-1 0.2s done
#1 creating container buildx_buildkit_inspiring_archimedes0
#1 30.03 error: failed to list workers: Unavailable: connection error: desc = "transport: error while dialing: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory"
#1 ERROR: exit code 1
------
 > [internal] booting buildkit:
 > [internal] booting buildkit:
30.03 error: failed to list workers: Unavailable: connection error: desc = "transport: error while dialing: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory"
ERROR: exit code 1
Command failed. Attempt 2/5:
Command failed. Attempt 2/5:
error: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory
ERROR: listing workers for Build: failed to list workers: Unavailable: connection error: desc = "error reading server preface: EOF"
Command failed. Attempt 3/5:
error: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory
ERROR: listing workers for Build: failed to list workers: Unavailable: connection error: desc = "error reading server preface: EOF"
Command failed. Attempt 4/5:
error: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory
ERROR: listing workers for Build: failed to list workers: Unavailable: connection error: desc = "error reading server preface: EOF"
Command failed. Attempt 5/5:
error: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory
ERROR: listing workers for Build: failed to list workers: Unavailable: connection error: desc = "error reading server preface: EOF"
##[error]Process completed with exit code 1.
Post job cleanup.

@fmease
Copy link
Member

fmease commented Feb 26, 2024

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2024
@bors
Copy link
Contributor

bors commented Feb 26, 2024

⌛ Testing commit 34eae07 with merge b79db43...

@bors
Copy link
Contributor

bors commented Feb 26, 2024

☀️ Test successful - checks-actions
Approved by: fmease
Pushing b79db43 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 26, 2024
@bors bors merged commit b79db43 into rust-lang:master Feb 26, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 26, 2024
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 26, 2024
…ease

Add `ErrorGuaranteed` to `ast::LitKind::Err`, `token::LitKind::Err`.

Similar to recent work doing the same for `ExprKind::Err` (rust-lang#120586) and `TyKind::Err` (rust-lang#121109).

r? `@oli-obk`
This was referenced Feb 26, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b79db43): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.1% [2.7%, 3.5%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.3% [-1.3%, -1.3%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 649.794s -> 649.972s (0.03%)
Artifact size: 311.12 MiB -> 311.16 MiB (0.01%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 26, 2024
Properly emit `expected ;` on `#[attr] expr`

Fixes rust-lang#121647

See rust-lang#118182, rust-lang#120586

---
r? estebank
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2024
Rollup merge of rust-lang#121651 - ShE3py:issue-121647, r=estebank

Properly emit `expected ;` on `#[attr] expr`

Fixes rust-lang#121647

See rust-lang#118182, rust-lang#120586

---
r? estebank
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 7, 2024
Add `ErrorGuaranteed` to `ast::ExprKind::Err`

See rust-lang#119967 for context
```
      \
       \
          _~^~^~_
      \) /  o o  \ (/
        '_   -   _'
        / '-----' \
```

r? fmease
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
Add `ErrorGuaranteed` to `ast::ExprKind::Err`

See rust-lang#119967 for context
```
      \
       \
          _~^~^~_
      \) /  o o  \ (/
        '_   -   _'
        / '-----' \
```

r? fmease
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants