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

Audit accesses to the source_map #97417

Open
cjgillot opened this issue May 26, 2022 · 2 comments
Open

Audit accesses to the source_map #97417

cjgillot opened this issue May 26, 2022 · 2 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-HIR Area: The high-level intermediate representation (HIR) A-incr-comp Area: Incremental compilation E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cjgillot
Copy link
Contributor

cjgillot commented May 26, 2022

The exact source code text in the SourceMap is not tracked for incremental compilation. Accessing it and changing behaviour depending on this text is bound to cause issues with incremental compilation. Accesses to the source_map should therefore be audited, to remove access to such untracked information.

As the main use for these is diagnostics (accessing the exact source code, adjusting a span...). As a consequence, we don't expect ICEs or miscompilations. The worse that could happen is weird diagnostics.

Are fine:

  • accesses that happen before incremental compilation kicks in: parsing, macro expansion, AST passes, lowering, name resolution...
  • accesses that are hashed in impl HashStable for Span: filename, line number, column number...
  • accesses in rustdoc, clippy, rustfmt, as they don't use rustc's incremental compilation.

Steps:

  1. create a query source_map: () -> &'tcx SourceMap, marked eval_always and no_hash;
  2. for each access to tcx.sess.source_map() (there are many), determine if it looks at the source code text;
    2.2 if it does, see below;
    2.1. if it does not: leave it as it, wrap this access in a method on TyCtxt in rustc_middle::ty::context and document why this access is fine.

When an untracked access to the source code is found, there are three options, in order:

  • try to replace the logic with one based on information available in the HIR;
  • add the required information to the HIR and use it, it's meant for this sort of things;
  • replace tcx.sess.source_map() by tcx.source_map(()), which will force the query system to recompute everything properly.

Note: there are many of such accesses. A quick grep counted ~300.
We do not expect a single contributor to audit all of them at once.

Do not hesitate to contact @cjgillot on zulip if necessary.

@cjgillot cjgillot added A-diagnostics Area: Messages for errors, warnings, and lints E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. A-HIR Area: The high-level intermediate representation (HIR) A-incr-comp Area: Incremental compilation E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels May 26, 2022
@mdx97
Copy link
Contributor

mdx97 commented May 27, 2022

@rustbot claim

@cjgillot
Copy link
Contributor Author

In addition: the compiler uses guess_head_span in many places to obtain a span which corresponds to the first line of an item. For instance the impl Trait for Foo in impl Trait for Foo { ... }.

We could add a head_span field to hir::Item, hir::TraitItem, hir::ImplItem and hir::ForeignItem which is set to sess.source_map().guess_head_span(item.span) (lowering happens before incremental compilation starts, so this is fine), or something more elaborate when available. The function tcx.hir().opt_span could then be modified to return this field. This would alleviate the need for many calls to guess_head_span in the codebase.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 5, 2022
…-and-impl-item, r=cjgillot

Replace some `guess_head_span` with `def_span`

This patch fixes a part of rust-lang#97417.
r? `@cjgillot`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 6, 2022
…-and-impl-item, r=cjgillot

Replace some `guess_head_span` with `def_span`

This patch fixes a part of rust-lang#97417.
r? `@cjgillot`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 3, 2022
…gillot

Keep track of the start of the argument block of a closure

This removes a call to `tcx.sess.source_map()` from [compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs](https://github.com/rust-lang/rust/compare/master...SarthakSingh31:issue-97417-1?expand=1#diff-8406bbc0d0b43d84c91b1933305df896ecdba0d1f9269e6744f13d87a2ab268a) as required by rust-lang#97417.

VsCode automatically applied `rustfmt` to the files I edited under `src/tools`. I can undo that if its a problem.

r? `@cjgillot`
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
…gillot

Keep track of the start of the argument block of a closure

This removes a call to `tcx.sess.source_map()` from [compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs](https://github.com/rust-lang/rust/compare/master...SarthakSingh31:issue-97417-1?expand=1#diff-8406bbc0d0b43d84c91b1933305df896ecdba0d1f9269e6744f13d87a2ab268a) as required by rust-lang#97417.

VsCode automatically applied `rustfmt` to the files I edited under `src/tools`. I can undo that if its a problem.

r? `@cjgillot`
calebcartwright pushed a commit to calebcartwright/rust that referenced this issue Jan 24, 2023
…gillot

Keep track of the start of the argument block of a closure

This removes a call to `tcx.sess.source_map()` from [compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs](https://github.com/rust-lang/rust/compare/master...SarthakSingh31:issue-97417-1?expand=1#diff-8406bbc0d0b43d84c91b1933305df896ecdba0d1f9269e6744f13d87a2ab268a) as required by rust-lang#97417.

VsCode automatically applied `rustfmt` to the files I edited under `src/tools`. I can undo that if its a problem.

r? `@cjgillot`
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-HIR Area: The high-level intermediate representation (HIR) A-incr-comp Area: Incremental compilation E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants