Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle Spans that reference imported SourceFiles #68941

Merged
merged 1 commit into from
Mar 19, 2020

Conversation

Aaron1011
Copy link
Member

Previously, metadata encoding used DUMMY_SP to represent any spans that
referenced an 'imported' SourceFile - e.g. a SourceFile from an upstream
dependency. This currently has no visible consequences, since these
kinds of spans don't currently seem to be emitted anywhere. However,
there's no reason that we couldn't start using such spans in
diagnostics.

This PR changes how we encode and decode spans in crate metadata. We
encode spans in one of two ways:

  • 'Local' spans, which reference non-imported SourceFiles, are encoded
    exactly as before.
  • 'Foreign' spans, which reference imported SourceFiles, are encoded
    with the CrateNum of their 'originating' crate. Additionally, their
    'lo' and 'high' values are rebased on top of the 'originating' crate,
    which allows them to be used with the SourceMap data encoded for that
    crate.

To support this change, I've also made the following modifications:

  • DefId and related structs are now moved to rustc_span. This allows
    us to use a CrateNum inside SourceFile. CrateNum has special
    handling during deserialization (it gets remapped to be the proper
    CrateNum from the point of view of the current compilation session),
    so using a CrateNum instead of a plain integer 'workaround type' helps
    to simplify deserialization.
  • The ExternalSource enum is renamed to ExternalSourceKind. There is
    now a struct called ExternalSource, which holds an
    ExternalSourceKind along with the original line number information for
    the file. This is used during Span serialization to rebase spans onto
    their 'owning' crate.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2020
@Aaron1011
Copy link
Member Author

Aaron1011 commented Feb 8, 2020

Unfortunately, I haven't found a way to actually test this PR. All of the ui tests are unchanged, which suggests that we don't emit any diagnostics using spans affected by this change.

PR #66364 might have its output affected by this, as some macro-related spans might end up getting used across several different crates.

This will also help with my proof-of-concept implementation of #68686, which causes us to serialize many more spans than we did before (when serializing ExpnData).

src/librustc_span/lib.rs Outdated Show resolved Hide resolved
@Aaron1011
Copy link
Member Author

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned cramertj Feb 8, 2020
@eddyb
Copy link
Member

eddyb commented Feb 8, 2020

DefId and related structs are now moved to rustc_span

Any chance you could only move CrateNum?

Either way, r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned eddyb Feb 8, 2020
@Aaron1011
Copy link
Member Author

@eddyb: I started off only moving CrateNum, but I seem to remember hitting some orphan impl issue that required me to move the rest of def_id.rs.

I think fixing #68686 will require this anyway, since ExpnData (from rustc_span) needs to be able to reference DefId.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

src/librustc_span/lib.rs Show resolved Hide resolved
src/librustc_span/lib.rs Show resolved Hide resolved
src/librustc_span/lib.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 8, 2020

(Review is still in progress, but I'll likely leave the rest of it to tomorrowsometime the next week.)

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Reviewed.
I'm not ready to merge this as written and need to think how to do this properly.

@petrochenkov
Copy link
Contributor

Something like #69432 should probably be a pre-requisite for foreign span decoding.

bors added a commit that referenced this pull request Feb 24, 2020
[experiment] rustc_metadata: Load metadata for indirect macro-only dependencies

Imagine this dependency chain between crates
```
Executable crate -> Library crate -> Macro crate
```
where "Library crate" uses the macros from "Macro crate" for some code generation, but doesn't reexport them any further.

Currently, when compiling "Executable crate" we don't even load metadata for it, because why would we want to load any metadata from "Macro crate" if it already did all its code generation job when compiling "Library crate".
Right?

Wrong!
Hygiene data and spans (#68686, #68941) from "Macro crate" still may need to be decoded from "Executable crate".
So we'll have to load them properly.

Questions:
- How this will affect compile times for larger crate trees in practice? How to measure it?
Hygiene/span encoding/decoding will necessarily slow down compilation because right now we just don't do some work that we should do, but this introduces a whole new way to slow down things. E.g. loading metadata for `syn` (and its dependencies) when compiling your executable if one of its library dependencies uses it.
- We are currently detecting whether a crate reexports macros from "Macro crate" or not, could we similarly detect whether a crate "reexports spans" and keep it unloaded if it doesn't?
Or at least "reexports important spans" affecting hygiene, we can probably lose spans that only affect diagnostics.
@Aaron1011
Copy link
Member Author

Aaron1011 commented Feb 24, 2020

@petrochenkov: Did you have additional concerns beyond what #69432 addresses?

@petrochenkov
Copy link
Contributor

@Aaron1011
SlowMetas is a pretty big hack that sets one more barrier on the way to removing Lrc and locks from the crate store.
Rustdoc is the other barrier, see comments on #65625 for some background. Crate store isn't really shared, Lrc is rather a way to make it reasonably cloneable.

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 13, 2020
@bors
Copy link
Contributor

bors commented Mar 14, 2020

☀️ Try build successful - checks-azure
Build commit: 5c45a31e73b566f9634237db352645edf24cef98 (5c45a31e73b566f9634237db352645edf24cef98)

@rust-timer
Copy link
Collaborator

Queued 5c45a31e73b566f9634237db352645edf24cef98 with parent 1572c43, future comparison URL.

@Aaron1011
Copy link
Member Author

Aaron1011 commented Mar 14, 2020

@petrochenkov: Perf looks like noise

@petrochenkov
Copy link
Contributor

Great!
@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2020

📌 Commit 76c7144 has been approved by petrochenkov

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 14, 2020
@petrochenkov petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2020
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Mar 17, 2020
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.
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 17, 2020
…ochenkov

Properly handle Spans that reference imported SourceFiles

Previously, metadata encoding used DUMMY_SP to represent any spans that
referenced an 'imported' SourceFile - e.g. a SourceFile from an upstream
dependency. This currently has no visible consequences, since these
kinds of spans don't currently seem to be emitted anywhere. However,
there's no reason that we couldn't start using such spans in
diagnostics.

This PR changes how we encode and decode spans in crate metadata. We
encode spans in one of two ways:

* 'Local' spans, which reference non-imported SourceFiles, are encoded
  exactly as before.
* 'Foreign' spans, which reference imported SourceFiles, are encoded
  with the CrateNum of their 'originating' crate. Additionally, their
'lo' and 'high' values are rebased on top of the 'originating' crate,
which allows them to be used with the SourceMap data encoded for that
crate.

To support this change, I've also made the following modifications:

* `DefId` and related structs are now moved to `rustc_span`. This allows
  us to use a `CrateNum` inside `SourceFile`. `CrateNum` has special
handling during deserialization (it gets remapped to be the proper
`CrateNum` from the point of view of the current compilation session),
so using a `CrateNum` instead of a plain integer 'workaround type' helps
to simplify deserialization.
* The `ExternalSource` enum is renamed to `ExternalSourceKind`. There is
now a struct called `ExternalSource`, which holds an
`ExternalSourceKind` along with the original line number information for
the file. This is used during `Span` serialization to rebase spans onto
their 'owning' crate.
@bors
Copy link
Contributor

bors commented Mar 19, 2020

☔ The latest upstream changes (presumably #70118) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 19, 2020
Previously, metadata encoding used DUMMY_SP to represent any spans that
referenced an 'imported' SourceFile - e.g. a SourceFile from an upstream
dependency. These leads to sub-optimal error messages in certain cases
(see the included test).

This PR changes how we encode and decode spans in crate metadata. We
encode spans in one of two ways:

* 'Local' spans, which reference non-imported SourceFiles, are encoded
  exactly as before.
* 'Foreign' spans, which reference imported SourceFiles, are encoded
  with the CrateNum of their 'originating' crate. Additionally, their
'lo' and 'high' values are rebased on top of the 'originating' crate,
which allows them to be used with the SourceMap data encoded for that
crate.

The `ExternalSource` enum is renamed to `ExternalSourceKind`. There is
now a struct called `ExternalSource`, which holds an
`ExternalSourceKind` along with the original line number information for
the file. This is used during `Span` serialization to rebase spans onto
their 'owning' crate.
@Aaron1011
Copy link
Member Author

@petrochenkov: Rebased

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2020

📌 Commit 5e28561 has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Mar 19, 2020

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

@bors 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 Mar 19, 2020
bors added a commit that referenced this pull request Mar 19, 2020
Rollup of 9 pull requests

Successful merges:

 - #68941 (Properly handle Spans that reference imported SourceFiles)
 - #69036 (rustc: don't resolve Instances which would produce malformed shims.)
 - #69443 (tidy: Better license checks.)
 - #69814 (Smaller and more correct generator codegen)
 - #69929 (Regenerate tables for Unicode 13.0.0)
 - #69959 (std: Don't abort process when printing panics in tests)
 - #69969 (unix: Set a guard page at the end of signal stacks)
 - #70005 ([rustdoc] Improve visibility for code blocks warnings)
 - #70088 (Use copy bound in atomic operations to generate simpler MIR)

Failed merges:

r? @ghost
@bors bors merged commit fddbee6 into rust-lang:master Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants