Store Ident in DefPathData instead of Symbol
#69906
Conversation
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| | ---- | ||
| | | | ||
| | not covered | ||
| | not covered |
There was a problem hiding this comment.
This is a little confusing, but it also occurs for enums defined in the same crate, so it isn't an issue with this PR.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Won't this regress incremental compilation, as changing the span of a definition will change it's |
|
@bjorn3: The |
|
@bjorn3: Note that we already associate a |
Can this new span be kept somewhere near that old span instead of what this PR does? The purpose of |
|
r? @eddyb |
The span is logically associated with the Storing the span somewhere else would either require us to duplicate the variants of
I agree that this isn't great. I'm going to see how much fallout there is from removing the |
| @@ -537,22 +560,22 @@ impl DefPathData { | |||
| } | |||
| } | |||
|
|
|||
| pub fn as_symbol(&self) -> Symbol { | |||
There was a problem hiding this comment.
If the direction with keeping the span in DefPathData is pursued, this method can be kept for compatibility and used in places where full identifier is not needed. That will significantly reduce the churn.
Aaron1011
force-pushed
the
feature/final-more-idents
branch
from
56592d8 to
8bd40ae
Compare
|
@petrochenkov: I removed the There is no longer any possibility of accidentally comparing two If we ever need to compare two arbitrary |
|
This is problematic for incremental compilation. You can't include |
|
@Zoxc: What would be the consequences of not hashing the |
|
☔ The latest upstream changes (presumably #70024) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Note that storing these #![feature(decl_macro)]
macro x() { struct X; }
x!();
x!();we need to encode the full This means that we actually do care about the Does storing the |
This allows us to recover span information when emitting cross crate errors. A number of test cases benefit from this change, since we were previously not emitting messages due to invalid spans. There are four main parts to this commit: 1. Adding `Ident` to `DefPathData`, and updating the affected code. This mostly consists of mechanical changes. 2. Updating how we determine the disambiguator for a `DefPath`. Since `DefPathData` now stores a `Span` (inside the `Ident`), we need to make sure we ignore this span we determining if a path needs to be disambiguated. This ensure that two paths with the same symbols but different spans are considered equal (this can occur when a macro is repeatedly expanded to a definition). 3. Ensuring that we are able to decode `Spans` when decoding the `DefPathTable`. Since the `DefPathTable` is stored in `CrateMetadata`, we must decode it before we have a `CrateMetadata` available. Since decoding a `Span` requires access to several fields from `CrateMetadata`, this commit adds a new struct `InitialCrateMetadata`, which implements `Metadata` and stores all of the necessary fields. 4. The specialized metadata encoder/decoder impls for `Ident` are removed. This causes us to fall back to the default encoder/decoder implementations for `Ident`, which simply serializes and deserializes the fields of `Ident`. This is strictly an improvement - we still don't have any hygiene information, but we now have a non-dummy Span. This should hopefully allow us to test PR rust-lang#68941, since we will now use cross-crate spans in more places.
Converting an `Ident` to a string will cause us to try to guess if the identifier is 'raw', potentially inserting a 'r#' prefix. However, all of the existing code that string-ifys `DefPathData` is expecting to be working with a `Symbol`, which never has a `r#` suffix. This was causing Rustdoc to generate links containing `r#` for structs/traits with raw identifiers (e.g. `r#struct`), leading to broken links when the actual html files were generated without the `r#` suffix.
This will cause an error if we ever add/change variants of `DefPathData`, as we need to ensure we strip spans from any `Ident`s
These are a footgun, since they will compare/hash based on the `Span` of any `Ident` in the `DefPathData`. We now use a private `PlainDefPathData` enum as the key for the `next_disambiguator` `HashMap`. This enum is the same as `DefPathData`, but it stores `Symbol` instead of `Ident` (e.g. what `DefPathData` used to store). This create a small amount of boilerplate, but allows the rest of the codebase to work exclusively with the normal `DefPathData` enum (which uses `Ident`s). All of the non-`HashMap` uses of `PartialEq` turned out to be comparisons against a constant, so `matches!` can be used instead of `==`.
While the fields are equivalent, we we constructing `Ident`s for things that weren't really identifiers (e.g `kw::Invalid` + the span of one of the types in a tuple struct).
This was pointless churn
Aaron1011
force-pushed
the
feature/final-more-idents
branch
from
3cb101e to
766a1d3
Compare
|
Storing a Span in a side table automatically does the correct thing: Only add a dependency when it is actually used. Storing an Ident in the DefPath requires hashing to ignore it to avoid a dependency everywhere and that also means that there is no dependency when there should be one. If the macro |
|
Funnily enough, there's already a |
|
@Aaron1011 Does your example result in two distinct |
|
@Zoxc: My example does create two distinct |
|
Closing in favor of #70077, which uses the side table approach. |
This allows us to recover span information when emitting cross crate
errors. A number of test cases benefit from this change, since we were
previously not emitting messages due to invalid spans.
There are four main parts to this commit:
Adding
IdenttoDefPathData, and updating the affected code. Thismostly consists of mechanical changes.
Updating how we determine the disambiguator for a
DefPath. SinceDefPathDatanow stores aSpan(inside theIdent), we need tomake sure we ignore this span we determining if a path needs to be
disambiguated. This ensure that two paths with the same symbols but
different spans are considered equal (this can occur when a macro is
repeatedly expanded to a definition).
Ensuring that we are able to decode
Spanswhen decoding theDefPathTable. Since theDefPathTableis stored inCrateMetadata, wemust decode it before we have a
CrateMetadataavailable. Sincedecoding a
Spanrequires access to several fields fromCrateMetadata, this commit adds a new structInitialCrateMetadata,which implements
Metadataand stores all of the necessary fields.The specialized metadata encoder/decoder impls for
Identareremoved. This causes us to fall back to the default encoder/decoder
implementations for
Ident, which simply serializes and deserializesthe fields of
Ident. This is strictly an improvement - we still don'thave any hygiene information, but we now have a non-dummy Span.
This should hopefully allow us to test PR #68941, since we will now use
cross-crate spans in more places.