Properly record metavar spans for other expansions other than TT by compiler-errors · Pull Request #134478 · rust-lang/rust

compiler-errors · 2024-12-18T18:02:00Z

This properly records metavar spans for nonterminals other than tokentree. This means that we operations like span.to(other_span) work correctly for macros. As you can see, other diagnostics involving metavars have improved as a result.

Fixes #132908
Alternative to #133270

cc @ehuss
cc @petrochenkov

rustbot · 2024-12-18T18:02:10Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

compiler-errors · 2024-12-18T19:52:54Z

@bors try @rust-timer queue

Properly record metavar spans for other expansions other than TT This properly records metavar spans for nonterminals other than tokentree. This means that we operations like `span.to(other_span)` work correctly for macros. As you can see, other diagnostics involving metavars have improved as a result. Fixes rust-lang#132908 Alternative to rust-lang#133270 cc `@ehuss` cc `@petrochenkov`

bors · 2024-12-18T19:54:06Z

⌛ Trying commit 6d29557 with merge 2409fbb...

bors · 2024-12-18T21:38:14Z

☀️ Try build successful - checks-actions
Build commit: 2409fbb (2409fbbcc15923c5c141a9d1bde8cba11429ef6f)

rust-timer · 2024-12-18T22:56:12Z

Finished benchmarking commit (2409fbb): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 3.2%]	56
Regressions ❌ (secondary)	4.3%	[0.2%, 15.6%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-1.1%, -0.2%]	6
All ❌✅ (primary)	0.5%	[0.2%, 3.2%]	56

Max RSS (memory usage)

Results (primary 4.8%, secondary 3.2%)

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)	4.8%	[4.8%, 4.8%]	1
Regressions ❌ (secondary)	3.8%	[2.5%, 6.3%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-3.2%, -3.2%]	1
All ❌✅ (primary)	4.8%	[4.8%, 4.8%]	1

Cycles

Results (primary 3.2%, secondary 0.3%)

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)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	16.2%	[15.2%, 17.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-5.6%, -2.1%]	15
All ❌✅ (primary)	3.2%	[3.2%, 3.2%]	1

Binary size

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

Bootstrap: 769.824s -> 771.972s (0.28%)
Artifact size: 330.35 MiB -> 330.32 MiB (-0.01%)

compiler-errors · 2024-12-18T22:58:16Z

unsurprisingly it wrecks incr comp

compiler-errors · 2024-12-18T23:11:15Z

Testing to see how much of it is due to the span hashing and how much is due to just recording the spans.

@bors try @rust-timer queue

bors · 2024-12-18T23:12:26Z

⌛ Trying commit b98ff91 with merge a429030...

Properly record metavar spans for other expansions other than TT This properly records metavar spans for nonterminals other than tokentree. This means that we operations like `span.to(other_span)` work correctly for macros. As you can see, other diagnostics involving metavars have improved as a result. Fixes rust-lang#132908 Alternative to rust-lang#133270 cc `@ehuss` cc `@petrochenkov`

bors · 2024-12-21T22:25:14Z

☀️ Try build successful - checks-actions
Build commit: 8764676 (8764676c47ec5f208e293efd2d7a712c7d01c8e3)

rust-timer · 2024-12-21T23:41:37Z

Finished benchmarking commit (8764676): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.4%]	26
Regressions ❌ (secondary)	0.9%	[0.1%, 3.0%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	5
All ❌✅ (primary)	0.2%	[0.1%, 0.4%]	26

Max RSS (memory usage)

Results (secondary 4.4%)

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)	5.3%	[1.6%, 13.0%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.3%, -2.4%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -4.2%, secondary 5.2%)

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)	5.2%	[3.1%, 6.1%]	4
Improvements ✅ (primary)	-4.2%	[-6.2%, -2.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-4.2%	[-6.2%, -2.2%]	2

Binary size

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

Bootstrap: 760.428s -> 761.338s (0.12%)
Artifact size: 330.62 MiB -> 330.61 MiB (-0.00%)

compiler-errors · 2024-12-21T23:53:30Z

I think the regressions are justified by this fix. I think we've actually ended up suppressing a lot of diagnostics in macros because of this not working, and it's evidently very useful for edition migrations too.

compiler-errors · 2024-12-21T23:53:34Z

@rustbot ready

traviscross · 2025-01-21T22:18:21Z

r? @oli-obk

traviscross · 2025-01-21T22:26:40Z

On the edition call, we discussed this one. This fixes something that we saw repeatedly when analyzing the crater runs for the edition. What happens isn't good; it produces something useless and breaks the migration. So getting this landed, if possible, would be good for supporting the upcoming edition in that way.

How might we feel about a backport here? Does it seem safe, or no?

oli-obk · 2025-01-22T08:22:56Z

@bors r+

bors · 2025-01-22T08:22:59Z

📌 Commit 2de21ad has been approved by oli-obk

It is now in the queue for this repository.

oli-obk

Just some thoughts, and something for me to investigate.

oli-obk · 2025-01-22T08:23:58Z

+
+    /// Freeze the set, and return the spans which have been read.
+    ///
+    /// After this is frozen, no spans that have not been read can be read.


Suggested change

/// After this is frozen, no spans that have not been read can be read.

/// After this is frozen, only spans that have already been read can be read.

oli-obk · 2025-01-22T08:25:14Z

+    ///
+    /// After this is frozen, no spans that have not been read can be read.
+    pub fn freeze_and_get_read_spans(&self) -> UnordMap<Span, Span> {
+        self.0.freeze().items().filter(|(_, (_, b))| *b).map(|(s1, (s2, _))| (*s1, *s2)).collect()


Do we ever use this twice? if not, could also poison the lock after extracting the spans we care about.

Poison the lock on the whole map?
freeze_and_get_read_spans is called once, but we still need to read the map after that, cannot panic on those later reads.

oli-obk · 2025-01-22T08:29:05Z

-            insert(mspans, dspan.open, metavar_span)
-                && insert(mspans, dspan.close, metavar_span)
-                && insert(mspans, dspan.entire(), metavar_span)
+            mspans.insert(dspan.open, metavar_span)
+                && mspans.insert(dspan.close, metavar_span)
+                && mspans.insert(dspan.entire(), metavar_span)


Orthogonal to this PR, but It's not clear to me that it's good keeping the previously inserted spans in the map if one of the later ones in this chain fails.

The more spans the better, so the best logic here would be

let b1 = mspans.insert(dspan.open, metavar_span); let b2 = mspans.insert(dspan.close, metavar_span); let b3 = mspans.insert(dspan.entire(), metavar_span); b1 && b2 && b3

But it probably doesn't matter anyway, because b{1,2,3} always (or almost always) have the same value in practice, and the metavar span retrieval is best effort anyway.

jieyouxu · 2025-01-22T12:34:01Z

@bors p=5 (rollup scheduling)

bors · 2025-01-22T14:46:45Z

⌛ Testing commit 2de21ad with merge dee7d0e...

bors · 2025-01-22T17:32:01Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing dee7d0e to master...

rust-timer · 2025-01-22T18:48:54Z

Finished benchmarking commit (dee7d0e): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.4%]	31
Regressions ❌ (secondary)	1.0%	[0.1%, 3.0%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	0.2%	[0.1%, 0.4%]	31

Max RSS (memory usage)

Results (primary -0.6%, secondary 5.4%)

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)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	6.3%	[0.4%, 10.9%]	17
Improvements ✅ (primary)	-1.6%	[-1.8%, -1.3%]	2
Improvements ✅ (secondary)	-2.2%	[-2.7%, -1.7%]	2
All ❌✅ (primary)	-0.6%	[-1.8%, 1.3%]	3

Cycles

Results (secondary 5.5%)

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)	5.5%	[5.2%, 5.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 765.273s -> 762.981s (-0.30%)
Artifact size: 325.95 MiB -> 325.94 MiB (-0.00%)

Kobzol · 2025-01-27T21:37:56Z

This regression was deemed justified, it was caused by a bugfix.

@rustbot label: +perf-regression-triaged

rustbot assigned SparrowLii Dec 18, 2024

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 18, 2024

compiler-errors mentioned this pull request Dec 18, 2024

Fix span of unsafe attribute diagnostic #133270

Closed