Remove E0773 "A builtin-macro was defined more than once."

[m-ou-se]

Error E0773 "A builtin-macro was defined more than once" is triggered when using the same #[rustc_builtin_macro(..)] twice. However, it can only be triggered in unstable code (using a rustc_ attribute), and there doesn't seem to be any harm in using the same implementation from compiler/rustc_builtin_macros/… for multiple macro definitions.

By changing the Box to an Arc in SyntaxExtensionKind, we can throw away the BuiltinMacroState::{NotYetSeen, AlreadySeen} logic, simplifying things.

[rustbot]

r? @cjgillot

rustbot has assigned @cjgillot.
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

[rustbot]

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

[m-ou-se]

cc @jyn514 who originally added this error in #76143

[jdonszelmann]

I reviewed the changes and they look reasonable to me. I just can't quite judge how important this check was. The check feels to me like just a sanity check, and there's now a good usecase for actually doing the thing the error tries to prevent.

[m-ou-se]

Originally, the macro was just removed from builtin_macros when #[rustc_builtin_macro] was used, resuting in a confusing "not found" error when used more than once.

To avoid that confusion, #76143 added logic to keep track of the already used builtin macros to give a better error. I think the better/simpler strategy is to just use Arc instead of Box such that you don't have to remove or track anything, lifting the 'only one definition' limitation.

[jdonszelmann]

right, so by making things an Arc, we solve the issue for which the error was initially added. Then I don't think there's much reason not to do this :)

[jdonszelmann]

alright then, first @bors r+ rollup here we go!

[bors]

📌 Commit 97d870e has been approved by jdonszelmann

It is now in the queue for this repository.

[jyn514]

i don't quite follow why it's useful to define a macro more than once, but as long as it works i have no objection. people shouldn't be using rustc_builtin_macro outside of std anyway.

[jyn514]

wait, you also removed the test? please don't do that, please test that duplicate macros work.

[jdonszelmann]

this is useful when you want multiple macros with different names implemented by the same definition in rustc_builtin_macros

[m-ou-se]

It's also useful when making a quick test/example while working on rustc, to not get conflicts with core when testing something with rustc_builtin_macro.

[m-ou-se]

i don't quite follow why it's useful to define a macro more than once

I don't see why it's useful to limit it to one use either. (Which caused some hard to debug issues in the past.)

[workingjubilee]

this is useful when you want multiple macros with different names implemented by the same definition in rustc_builtin_macros

Is there a problem with having the other macros forward to that builtin one?

[m-ou-se]

Is there a problem with having the other macros forward to that builtin one?

That's not as easy for attribute and derive macros.

[workingjubilee]

Ooh, yeah. Doable but still, gotcha.

[jdonszelmann]

@jyn514 's comment of adding a test sounds like a good one. After that I'm willing to r+ again :)

[fmease]

could you also squash :)

@bors r-

[petrochenkov]

The Box -> Arc change doesn't affect just built-in macros, it affects all macros, so let's check the perf first.
@bors try @rust-timer queue

[bors]

⌛ Trying commit c9eda62 with merge cfeb465...

[bors]

☀️ Try build successful - checks-actions
Build commit: cfeb465 (cfeb4659d0a4e3cac73a43d4b6e3b8b4c08da529)

[rust-timer]

Finished benchmarking commit (cfeb465): comparison URL.

Overall result: ❌ regressions - no action needed

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.

@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
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.5%, secondary -1.8%)

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)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.7%, -2.1%]	3
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	1

Cycles

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

Binary size

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

Bootstrap: 776.359s -> 775.772s (-0.08%)
Artifact size: 365.12 MiB -> 365.09 MiB (-0.01%)

[petrochenkov]

The change itself makes sense.
r=me after restoring the comment (#138613 (comment)) and tests (#138613 (comment)), and squashing commits.
@rustbot author
@bors rollup=maybe

[jdonszelmann]

@bors r=jdonszelmann,petrochenkov then :)

[bors]

📌 Commit 6c865c1 has been approved by jdonszelmann,petrochenkov

It is now in the queue for this repository.