-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Description
The experiment in #125107 uncovered an interesting source of version breakage, which occurs if the standard library prelude attempts to introduce an additional type.
Suppose std
adds the type Cell
to the prelude. This will break code that looks like this:
use Cell::A;
// The derive here is important but it doesn't particularly matter which trait is being derived.
#[derive(PartialEq)]
pub enum Cell {
A
}
Per the comment at #125107 (comment) , this generates an error of the form:
error[E0659]: `Cell` is ambiguous
--> src/puzzle.rs:5:5
|
5 | use Cell::{Empty, Unknown, ValA, ValB, ValC, ValD};
| ^^^^ ambiguous name
|
= note: ambiguous because of a conflict between a macro-expanded name and a less macro-expanded name from outer scope during import or macro resolution
note: `Cell` could refer to the enum defined here
--> src/puzzle.rs:8:1
|
8 | / pub enum Cell {
9 | | ValA,
10 | | ValB,
11 | | ValC,
... |
14 | | Unknown,
15 | | }
| |_^
= help: use `self::Cell` to refer to this enum unambiguously
note: `Cell` could also refer to a struct from prelude
--> /rustc/9130c02509ce15f69dc5da6359bb9d140d41d4ac/library/std/src/prelude/mod.rs:148:13
This makes it a potentially breaking change to add new types to the prelude. We could still potentially add types that create no actual breakage (e.g. because no extant Rust code declares its own conflicting type), but this nonetheless introduces a potential source of breakage that could block introducing new types until an edition boundary (and creates one more thing people have to deal with when migrating to the new edition).
It seems worth exploring whether we can, or should, attempt to fix this, such as by allowing the type to shadow in this case. Note that it does properly shadow if there's no derive
on the enum.
Nominating for lang discussion on the "should we" question: should we consider doing something to eliminate or mitigate this source of conflict? This kind of conflict applies both to the current problem with the standard library prelude, as well as any potential future features for letting other libraries have preludes.
Nominating for compiler discussion on the "can we" question: to find out how feasible it would be to deprioritize standard library prelude types and make them lower priority than local enums in this case, or to otherwise mitigate the issue that seems to specifically arise when using a derive
macro on such types.
Activity
Noratrieb commentedon Jul 15, 2024
@petrochenkov do you know whether this is possible? (I suspect the people in the compiler meeting will not know)
petrochenkov commentedon Jul 15, 2024
@Nilstrieb
What exactly, removing the "
Cell
is ambiguous" error?It is a part of the "time travel prevention" machinery in macro/import resolution that ensures that resolution results are independent of things like order of items in the crate, or some specific expansion order used by the compiler.
The issue is not related to
derive
specifically, it would also appear ifenum Cell
were emitted by any other macro.In the specific example above, I don't think the error can be avoided.
The use of
Cell
(use Cell::A
) does materialize earlier than its definition (enum Cell
) emitted by thederive
macro, so we must restrict shadowing.joshtriplett commentedon Jul 16, 2024
@petrochenkov I really appreciate the pointer to that writeup, thank you. I think I understand why we restrict shadowing in that case.
So, if I'm understanding correctly, the ordering problem is roughly:
Cell
in the prelude.use Cell::A;
.derive
).use Cell::A;
will refer to theCell
we've seen, from the prelude.Cell
, which would shadow the one in the prelude.use Cell::A;
could refer to the definition just produced by the macro expansion in the first pass, so we error.Is that an accurate description?
And, in the case where we don't have a
derive
, both definitions come from the first pass, so we allow one to shadow the other?I understand that we can't fix the fully general case of that. Would it be possible to mitigate this issue in the common case of a
derive
? We know that aderive
(even a proc-macro derive) applied to a top-level itemX
can't change that top-level item, it can only add additional things. So, given that, could we arrange to know in the first pass that we have aCell
, even if it has aderive
attached to it? That would then let us fall into the same case as if we didn't have the proc macro, where all the names showed up in the first pass alongside theuse
.petrochenkov commentedon Jul 17, 2024
Yes.
When we see
use Cell::A
we have two choicesCell
that we see right now (the prelude one), even if it can be potentially shadowed laterspecific_module::Cell
), then we can block and wait instead of possibly producing restricted shadowing errors laterpetrochenkov commentedon Jul 17, 2024
Only if
derive
is turned from a macro into a built-in syntax.Basically, the main requirement for this is that
derive
should not go through name resolution.#[derive]
we immediately know that it is indeed the well known https://doc.rust-lang.org/stable/std/prelude/rust_2024/attr.derive.html, then we can immediately expand it and produceenum Cell
in the "first pass".derive
instd::prelude::rust_2024
where it normally resides, or in some other place if it's shadowed, then it can potentially send the derive's expansion to "second/third/etc pass" even if it ends up being the standard well knownderive
eventually.petrochenkov commentedon Jul 17, 2024
It may be possible to amend macro expansion algorithm with "immediate expansion" attempts.
If we see a macro invocation (that includes
#[derive]
) and an immediate attempt at resolving it succeeds, then we immediately expand it as well, instead of putting it into the usual queue.This should lift some restrictions from code emitted by such macro, including shadowing restrictions like in the example above.
In practice most
derive
invocations will be able to be promoted to "immediate expansions".Not sure how viable this idea is, someone needs to try prototyping.
petrochenkov commentedon Jul 17, 2024
On a second thought, results of a probing like this are going depend on the specific expansion order, and that's what we are trying to avoid now.
joshtriplett commentedon Jul 22, 2024
@petrochenkov
That sounds potentially feasible, and seems unlikely to create conflicts. I'd be genuinely surprised if anything in the ecosystem is overriding the name
derive
.petrochenkov commentedon Jul 22, 2024
I've re-read the code in
compiler\rustc_builtin_macros\src\derive.rs
andfn resolve_derives
.#[derive(Foo, Bar)] enum Cell { ... }
doesn't emitenum Cell { ... }
immediately, it resolvesFoo
andBar
first, which may delay it to a "second pass".It's necessary to resolve derive helper attributes in
enum Cell { ... }
correctly, see comments infn resolve_derives
.In general I'd personally not want to go this way, the "macrofication" of
derive
fixed quite a number of issues, I wouldn't want to reintroduce them back or workaround them with hacks.What is the root motivation for this in the first place?
There are few names that are used universally enough to be added to prelude, and extending the prelude with new names is a major decision.
It's quite reasonable for it to happen once in 3 years on an edition bump.
joshtriplett commentedon Jul 29, 2024
I wasn't aware of that; thanks for that context.
Is there no reasonable low-friction way to keep the macro-based
derive
while teaching the compiler that thatderive
will definitely end up emitting the named item (e.g. a derive onCell
will definitely produce aCell
)?If that would still be too much of a hack, we can work around it. In any case I appreciate you considering the possibilities, thank you.
The point is to make it less of a major decision. There are many names that we may want to add to the prelude, and if it's possible to do so incrementally without waiting for an edition that would make it a smaller impact. It'd also reduce the amount of burden at edition time; anything we can make not require an edition means the edition becomes less work, and we've already had long conversations about how to make the edition less work.
Even with this limitation, we can still potentially add prelude types without an edition, if we do a crater run. And the fact that the conflict only occurs with the combination of a derive and an import of a macro variant means it's unlikely to arise often.
I'm trying to find out if there's any reasonable step we can take to further reduce the likelihood of such problems. If not, then we can document this limitation and work around it with additional care when extending the prelude.
joshtriplett commentedon Aug 24, 2024
We discussed this in today's @rust-lang/lang meeting for information's sake, and came to the conclusion that while we'd like to be able to make this situation more robust, we didn't have an obvious third option to suggest, and we don't want to place substantially more complexity on the compiler. So, if the compiler team feels this would add too much complexity, we'll have to continue leaving this in the realm of "be careful and check crater" for libs-api.