Remove `token::Lit` from `ast::MetaItemLit`. by nnethercote · Pull Request #105160 · rust-lang/rust

nnethercote · 2022-12-02T02:49:03Z

Currently ast::MetaItemLit represents the literal kind twice. This PR removes that redundancy. Best reviewed one commit at a time.

r? @petrochenkov

This makes it clearer that it's not a lossless conversion, which I find helpful.

This is required to distinguish between cooked and raw byte string literals in an `ast::LitKind`, without referring to an adjacent `token::Lit`. It's a prerequisite for the next commit.

rustbot · 2022-12-02T02:49:11Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

`token::Lit` contains a `kind` field that indicates what kind of literal it is. `ast::MetaItemLit` currently wraps a `token::Lit` but also has its own `kind` field. This means that `ast::MetaItemLit` encodes the literal kind in two different ways. This commit changes `ast::MetaItemLit` so it no longer wraps `token::Lit`. It now contains the `symbol` and `suffix` fields from `token::Lit`, but not the `kind` field, eliminating the redundancy.

To better match `MetaItemLit`.

compiler/rustc_ast/src/attr/mod.rs

compiler/rustc_expand/src/build.rs

compiler/rustc_parse/src/parser/expr.rs

compiler/rustc_parse/src/parser/pat.rs

compiler/rustc_ast/src/util/literal.rs

There are better ways to create the meta items. - In the rustdoc tests, the commit adds `dummy_meta_item_name_value`, which matches the existing `dummy_meta_item_word` function and `dummy_meta_item_list` macro. - In `types.rs` the commit clones the existing meta item and then modifies the clone.

rustbot · 2022-12-05T00:50:49Z

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

These two methods both produce a `MetaItemLit`, and then some of the call sites convert the `MetaItemLit` to a `token::Lit` with `as_token_lit`. This commit parameterises these two methods with a `mk_lit_char` closure, which can be used to produce either `MetaItemLit` or `token::Lit` directly as necessary.

It has a single call site in the HIR pretty printer, where the resulting token lit is immediately converted to a string. This commit replaces `LitKind::synthesize_token_lit` with a `Display` impl for `LitKind`, which can be used by the HIR pretty printer.

nnethercote · 2022-12-05T05:35:12Z

I have added five new commits addressing the review comments.

petrochenkov · 2022-12-05T15:19:02Z

@bors r+

bors · 2022-12-05T15:19:05Z

📌 Commit 7e0c6db has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2022-12-07T09:10:38Z

⌛ Testing commit 7e0c6db with merge f9cf4381d57f19593c5669da52315f7dce06b33f...

bors · 2022-12-07T09:30:36Z

💔 Test failed - checks-actions

nnethercote · 2022-12-08T21:29:34Z

This Zulip thread indicates that this coverage-reports failure is happening to other people as well.

richkadel · 2022-12-09T01:13:34Z

Good to know it's unrelated to your change. make is really not good but it was the only tool available other than writing something from scratch. I haven't been working in the Rust codebase in a while so hopefully someone with more recent/relevant knowledge will figure out and fix the Makefile compatibility issues.

nnethercote · 2022-12-09T04:46:17Z

#105477 has hopefully fixed the coverage-reports failure.

@bors r=petrochenkov

bors · 2022-12-09T04:46:19Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: Rollup of 10 pull requests #105456

bors · 2022-12-09T04:46:20Z

📌 Commit 7e0c6db has been approved by petrochenkov

It is now in the queue for this repository.

rust-log-analyzer · 2022-12-09T06:45:03Z

The job x86_64-msvc-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)


test result: FAILED. 46 passed; 1 failed; 29 ignored; 0 measured; 0 filtered out; finished in 8.07s

Build completed unsuccessfully in 0:55:18
make: *** [Makefile:73: ci-subset-1] Error 1

…trochenkov Remove `token::Lit` from `ast::MetaItemLit`. Currently `ast::MetaItemLit` represents the literal kind twice. This PR removes that redundancy. Best reviewed one commit at a time. r? `@petrochenkov`

…trochenkov Remove `token::Lit` from `ast::MetaItemLit`. Currently `ast::MetaItemLit` represents the literal kind twice. This PR removes that redundancy. Best reviewed one commit at a time. r? ``@petrochenkov``

…trochenkov Remove `token::Lit` from `ast::MetaItemLit`. Currently `ast::MetaItemLit` represents the literal kind twice. This PR removes that redundancy. Best reviewed one commit at a time. r? ```@petrochenkov```

bors · 2022-12-12T05:16:53Z

⌛ Testing commit 7e0c6db with merge 2cd2070...

bors · 2022-12-12T07:57:34Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 2cd2070 to master...

rust-timer · 2022-12-12T09:16:54Z

Finished benchmarking commit (2cd2070): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.0%	[1.0%, 1.0%]	1
Improvements ✅ (primary)	-0.9%	[-1.0%, -0.8%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-1.0%, -0.8%]	5

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)	-	-	0
Improvements ✅ (primary)	-1.8%	[-2.8%, -0.8%]	2
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-1.8%	[-2.8%, -0.8%]	2

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)	-1.7%	[-1.9%, -1.6%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.7%	[-1.9%, -1.6%]	4

nnethercote · 2022-12-12T09:39:46Z

There is a single trivial regression, which may or may not be real.

@rustbot label: +perf-regression-triaged

…ochenkov Remove `token::Lit` from `ast::MetaItemLit`. Currently `ast::MetaItemLit` represents the literal kind twice. This PR removes that redundancy. Best reviewed one commit at a time. r? `@petrochenkov`

nnethercote added 2 commits December 2, 2022 10:23

Rename LitKind::to_token_lit as LitKind::synthesize_token_lit.

e658144

This makes it clearer that it's not a lossless conversion, which I find helpful.

Add StrStyle to ast::LitKind::ByteStr.

a7f35c4

This is required to distinguish between cooked and raw byte string literals in an `ast::LitKind`, without referring to an adjacent `token::Lit`. It's a prerequisite for the next commit.

rustbot assigned petrochenkov Dec 2, 2022

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 Dec 2, 2022

nnethercote added 2 commits December 2, 2022 13:49

Reorder StrLit fields.

d5526ff

To better match `MetaItemLit`.

nnethercote force-pushed the rm-Lit-token_lit branch from fca81cc to d5526ff Compare December 2, 2022 02:50

petrochenkov reviewed Dec 2, 2022

View reviewed changes

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 Dec 2, 2022

nnethercote added 4 commits December 5, 2022 14:20

Remove ExtCtxt::expr_lit.

4ae956f

Remove three uses of LitKind::synthesize_token_lit.

568e647

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 Dec 5, 2022

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 Dec 5, 2022

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 Dec 7, 2022

RalfJung mentioned this pull request Dec 8, 2022

coverage-reports keeps failing on Windows (error mentions clear_expected_if_blessed) #105475

Closed

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 Dec 9, 2022

Dylan-DPC mentioned this pull request Dec 9, 2022

Rollup of 7 pull requests #105492

Closed

Dylan-DPC mentioned this pull request Dec 11, 2022

Rollup of 6 pull requests #105551

Closed

Dylan-DPC mentioned this pull request Dec 11, 2022

Rollup of 6 pull requests #105565

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 12, 2022

bors merged commit 2cd2070 into rust-lang:master Dec 12, 2022

rustbot added this to the 1.68.0 milestone Dec 12, 2022

rustbot added the perf-regression Performance regression. label Dec 12, 2022

nnethercote deleted the rm-Lit-token_lit branch December 12, 2022 09:39

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 12, 2022

Uh oh!

Conversation

nnethercote commented Dec 2, 2022

Uh oh!

rustbot commented Dec 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustbot commented Dec 5, 2022

Uh oh!

nnethercote commented Dec 5, 2022

Uh oh!

petrochenkov commented Dec 5, 2022

Uh oh!

bors commented Dec 5, 2022

Uh oh!

bors commented Dec 7, 2022

Uh oh!

bors commented Dec 7, 2022

Uh oh!

nnethercote commented Dec 8, 2022

Uh oh!

richkadel commented Dec 9, 2022

Uh oh!

nnethercote commented Dec 9, 2022

Uh oh!

bors commented Dec 9, 2022

Uh oh!

bors commented Dec 9, 2022

Uh oh!

rust-log-analyzer commented Dec 9, 2022

Uh oh!

bors commented Dec 12, 2022

Uh oh!

bors commented Dec 12, 2022

Uh oh!

rust-timer commented Dec 12, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Uh oh!

nnethercote commented Dec 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants