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

Dummy tweaks (attempt 2) #121829

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Dummy tweaks (attempt 2) #121829

merged 2 commits into from
Mar 6, 2024

Conversation

nnethercote
Copy link
Contributor

@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 Mar 1, 2024
@nnethercote
Copy link
Contributor Author

@petrochenkov: you added the DummyAstNode in #91519, it doesn't seem to be necessary any more, do you think we still need it?

@petrochenkov
Copy link
Contributor

it doesn't seem to be necessary any more, do you think we still need it?

Removing it seems brittle.
All kinds of work may happen inside those visit_clobbers - macro expansion (including proc macros perhaps?), name resolution.
I don't have much confidence that it can never report fatal errors, and even if they are not reported now we should not restrict that logic from reporting them in the future.
@rustbot author

@rustbot rustbot 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 Mar 1, 2024
@nnethercote
Copy link
Contributor Author

Ok, I have removed the old second commit that removes DummyAstNode. I have added two new commits.

@petrochenkov petrochenkov 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 Mar 2, 2024
@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 4, 2024

They are two different ways of creating dummy results, with two different purposes. Their implementations are separate except for crates, where DummyResult depends on DummyAstNode.

They are exactly the same purpose though, create some node that will just sit in AST during error recovery.

The fn raw_* methods could very well use DummyAstNode.
Spans can then be set at use site (is setting spans even useful there?) or DummyAstNode methods could grow span parameters.
@rustbot author

UPD: to clarify, the trait renaming also doesn't look useful to me.

@rustbot rustbot 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 Mar 4, 2024
@nnethercote
Copy link
Contributor Author

DummyAstNode and DummyResult are quite different if you look closely.

  • DummyAstNode is a trait, but DummyResult is a type.
  • For expressions, DummyAstNode uses ExprKind::Dummy, but DummyResult uses ExprKind::Err or an empty ExprKind::Tup
  • For types, DummyAstNode uses TyKind::Dummy, but DummyResult uses an empty TyKind::Tup.
  • DummyAstNode can handle a single Item, Stmt, Block, or Crate, but DummyResult cannot.
  • DummyResult can produce multiple items, statements, field, etc., but DummyAstNode cannot.

So the purpose is much the same, but the details vary greatly. This is why I removed the interdependency. You could probably frankenstein them into a single thing (trait? type?) that can be used in both cases, by adding extra parameters, but I think that would end up being less clear.

They are two different ways of creating dummy results, with two
different purposes. Their implementations are separate except for
crates, where `DummyResult` depends on `DummyAstNode`.

This commit removes that dependency, so they are now fully separate. It
also expands the comment on `DummyAstNode`.
@nnethercote
Copy link
Contributor Author

I have removed the third commit. I think the first two commits are still worth doing, and that DummyAstNode and DummyResult should not be combined, for the reasons described above.

@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 Mar 5, 2024
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 5, 2024

📌 Commit a9dff2d has been approved by petrochenkov

It is now in the queue for this repository.

@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 Mar 5, 2024
@petrochenkov
Copy link
Contributor

I've just learned about {ExprKind,TyKind}::Dummy, which is a very recent addition. (WWIC?!)

Considering the reasons for its introduction, fundamentally there should be 3 flavors of dummy AST nodes:

  • Error nodes, if we have a ErrorGuaranteed proof
    • This is the majority of cases, visit_clobber recovery case belongs here, MacResult for DummyResult also belongs here
  • Non-error dummy nodes, like DummyResult::any_valid, results of log_syntax!(), representation of impl Trait for .. {}
    • These are rare corner cases
  • Placeholder nodes during macro expansion (compiler\rustc_expand\src\placeholders.rs)
    • The NodeId/is_placeholder/*Kind::MacCall part there is not dummy, but everything else is unused and dummy
    • This is also commonly used

Both DummyAstNode and DummyResult would ideally be normalized for that.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…ochenkov

Dummy tweaks (attempt 2)

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…ochenkov

Dummy tweaks (attempt 2)

r? ``@petrochenkov``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…ochenkov

Dummy tweaks (attempt 2)

r? ```@petrochenkov```
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#121065 (Add basic i18n guidance for `Display`)
 - rust-lang#121301 (errors: share `SilentEmitter` between rustc and rustfmt)
 - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrate check)
 - rust-lang#121829 (Dummy tweaks (attempt 2))
 - rust-lang#121857 (Implement async closure signature deduction)
 - rust-lang#121894 (const_eval_select: make it safe but be careful with what we expose on stable for now)
 - rust-lang#121905 (Add a `description` field to target definitions)
 - rust-lang#122022 (loongarch: add frecipe and relax target feature)
 - rust-lang#122028 (Remove some dead code)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…ochenkov

Dummy tweaks (attempt 2)

r? ````@petrochenkov````
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#121065 (Add basic i18n guidance for `Display`)
 - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrate check)
 - rust-lang#121829 (Dummy tweaks (attempt 2))
 - rust-lang#121832 (Add new Tier-3 target: `loongarch64-unknown-linux-musl`)
 - rust-lang#121857 (Implement async closure signature deduction)
 - rust-lang#121894 (const_eval_select: make it safe but be careful with what we expose on stable for now)
 - rust-lang#122014 (Change some attributes to only_local.)
 - rust-lang#122016 (will_wake tests fail on Miri and that is expected)
 - rust-lang#122018 (only set noalias on Box with the global allocator)
 - rust-lang#122028 (Remove some dead code)

r? `@ghost`
`@rustbot` modify labels: rollup
@nnethercote
Copy link
Contributor Author

Those special node kinds have evolved over time, and could certainly be adjusted and made more uniform. ast::PatKind is an interesting case: it has an Err(ErrorGuaranteed) variant but no dummy variant, because PatKind::Wild has been good enough.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#121065 (Add basic i18n guidance for `Display`)
 - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrate check)
 - rust-lang#121829 (Dummy tweaks (attempt 2))
 - rust-lang#121857 (Implement async closure signature deduction)
 - rust-lang#121894 (const_eval_select: make it safe but be careful with what we expose on stable for now)
 - rust-lang#122014 (Change some attributes to only_local.)
 - rust-lang#122016 (will_wake tests fail on Miri and that is expected)
 - rust-lang#122018 (only set noalias on Box with the global allocator)
 - rust-lang#122028 (Remove some dead code)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#121065 (Add basic i18n guidance for `Display`)
 - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrate check)
 - rust-lang#121829 (Dummy tweaks (attempt 2))
 - rust-lang#121857 (Implement async closure signature deduction)
 - rust-lang#121894 (const_eval_select: make it safe but be careful with what we expose on stable for now)
 - rust-lang#122014 (Change some attributes to only_local.)
 - rust-lang#122016 (will_wake tests fail on Miri and that is expected)
 - rust-lang#122018 (only set noalias on Box with the global allocator)
 - rust-lang#122028 (Remove some dead code)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0783a63 into rust-lang:master Mar 6, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2024
Rollup merge of rust-lang#121829 - nnethercote:dummy-tweaks-2, r=petrochenkov

Dummy tweaks (attempt 2)

r? `````@petrochenkov`````
@nnethercote nnethercote deleted the dummy-tweaks-2 branch March 6, 2024 04:48
@fmease
Copy link
Member

fmease commented Mar 6, 2024

Sorry @petrochenkov for not including you in the discussions around Dummy, I should've pinged you. IIRC compiler-errors originally suggested it, or was it ShE3py (?), and I agreed that is was a good idea for now and gave the green light. Oli-obk and nnethercote were also involved in some discussions relating to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

6 participants