Skip to content

Conversation

fmease
Copy link
Member

@fmease fmease commented Sep 8, 2025

  • Stop stripping frontmatter in proc_macro::Literal::from_str (RUST-146132)
  • Stop stripping frontmatter in expr-ctxt (but not item-ctxt!) includes (RUST-145945)
  • Stop stripping shebang (!) in proc_macro::Literal::from_str
    • Not a breaking change because it did compare spans already to ensure there wasn't extra whitespace or comments (Literal::from_str("#!\n0") already yields Err(_) thankfully!)
  • Stop stripping frontmatter+shebang inside some rustdoc code where it doesn't make any observable difference (see self review comments)
  • (Stop stripping frontmatter+shebang inside internal test code)

Fixes #145945.
Fixes #146132.

r? fee1-dead

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue. labels Sep 8, 2025
@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the frontmatter-containment branch from 7f38e98 to 6071c21 Compare September 8, 2025 17:33
@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the frontmatter-containment branch from 6071c21 to 50f74e5 Compare September 8, 2025 18:05
@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the frontmatter-containment branch from 50f74e5 to 96edbd6 Compare September 9, 2025 15:17
@rust-log-analyzer

This comment has been minimized.

self.psess(),
name,
s.to_owned(),
StripTokens::Nothing,
Copy link
Member Author

@fmease fmease Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating this to not strip shebangs isn't breaking.

It already compares spans below to ensure no extra trivia is contained like whitespace, comments or – well – shebangs. However, it's just cleaner to pass Nothing here.

///
/// On failure, the errors must be consumed via `unwrap_or_emit_fatal`, `emit`, `cancel`,
/// etc., otherwise a panic will occur when they are dropped.
pub fn new_parser_from_simple_source_str(
Copy link
Member Author

@fmease fmease Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this convenience wrapper (for StripTokens::Nothing) to force all callers to have to think what behavior they want which is a lot more robust. Otherwise, people will just overlook this function and blindly go for the "normal" variant new_parser_from_source_str.

psess: &ParseSess,
name: FileName,
source: String,
override_span: Option<Span>,
Copy link
Member Author

@fmease fmease Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might add strip_tokens: StripTokens as a parameter to this function in a future PR. At the time of writing, it wouldn't really matter but there are some call sites like the one for -Zcrate-attr where we could pass StripTokens::Nothing (since the input is wrapped in #![ ] anyway) but it wouldn't lead to any observable difference in rustc's behavior.

&psess,
file_name,
snippet.clone(),
StripTokens::Nothing,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't make a difference since this is for (re)parsing macro matchers which will have already been validated by rustc and shebangs or frontmatters aren't part of that subgrammar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just test code; use StripTokens::Nothing for simplicity. If anybody wants to test shebangs and/or frontmatter in here, they can just update it.

};
// Don't strip any tokens; it wouldn't matter anyway because the source is wrapped in a function.
let mut parser =
match new_parser_from_source_str(&psess, filename, wrapped_source, StripTokens::Nothing) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change is unobservable as the added comment explains.

let parser = rustc_parse::unwrap_or_emit_fatal(rustc_parse::new_parser_from_source_str(
psess,
FileName::anon_source_code(source_code),
rustc_parse::lexer::StripTokens::Nothing,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also just test code.

@fmease fmease force-pushed the frontmatter-containment branch from 96edbd6 to 541e36f Compare September 9, 2025 16:22
@fmease fmease marked this pull request as ready for review September 9, 2025 16:22
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2025
Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

View changes since this review

@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the frontmatter-containment branch from 541e36f to 7a66925 Compare September 9, 2025 17:49
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, r=me when green

View changes since this review

@fmease
Copy link
Member Author

fmease commented Sep 9, 2025

@bors r=fee1-dead,Urgau rollup

@bors
Copy link
Collaborator

bors commented Sep 9, 2025

📌 Commit 7a66925 has been approved by fee1-dead,Urgau

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 9, 2025
…fee1-dead,Urgau

Strip frontmatter in fewer places

* Stop stripping frontmatter in `proc_macro::Literal::from_str` (RUST-146132)
* Stop stripping frontmatter in expr-ctxt (but not item-ctxt!) `include`s (RUST-145945)
* Stop stripping shebang (!) in `proc_macro::Literal::from_str`
  * Not a breaking change because it did compare spans already to ensure there wasn't extra whitespace or comments (`Literal::from_str("#!\n0")` already yields `Err(_)` thankfully!)
* Stop stripping frontmatter+shebang inside some rustdoc code where it doesn't make any observable difference (see self review comments)
* (Stop stripping frontmatter+shebang inside internal test code)

Fixes rust-lang#145945.
Fixes rust-lang#146132.

r? fee1-dead
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 9, 2025
…fee1-dead,Urgau

Strip frontmatter in fewer places

* Stop stripping frontmatter in `proc_macro::Literal::from_str` (RUST-146132)
* Stop stripping frontmatter in expr-ctxt (but not item-ctxt!) `include`s (RUST-145945)
* Stop stripping shebang (!) in `proc_macro::Literal::from_str`
  * Not a breaking change because it did compare spans already to ensure there wasn't extra whitespace or comments (`Literal::from_str("#!\n0")` already yields `Err(_)` thankfully!)
* Stop stripping frontmatter+shebang inside some rustdoc code where it doesn't make any observable difference (see self review comments)
* (Stop stripping frontmatter+shebang inside internal test code)

Fixes rust-lang#145945.
Fixes rust-lang#146132.

r? fee1-dead
bors added a commit that referenced this pull request Sep 10, 2025
Rollup of 6 pull requests

Successful merges:

 - #146311 (Minor symbol comment fixes.)
 - #146340 (Strip frontmatter in fewer places)
 - #146342 (Improve C-variadic error messages: part 2)
 - #146347 (report duplicate symbols added by the driver)
 - #146374 (Update `browser-ui-test` version to `0.22.2`)
 - #146379 (Fix `compare_against_sw_vers` test)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Sep 10, 2025
Rollup of 8 pull requests

Successful merges:

 - #145327 (std: make address resolution weirdness local to SGX)
 - #145879 (default auto traits: use default supertraits instead of `Self: Trait` bounds on associated items)
 - #146123 (Suggest examples of format specifiers in error messages)
 - #146311 (Minor symbol comment fixes.)
 - #146322 (Make Barrier RefUnwindSafe again)
 - #146327 (Add tests for deref on pin)
 - #146340 (Strip frontmatter in fewer places)
 - #146342 (Improve C-variadic error messages: part 2)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 86d39a0 into rust-lang:master Sep 11, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Sep 11, 2025
rust-timer added a commit that referenced this pull request Sep 11, 2025
Rollup merge of #146340 - fmease:frontmatter-containment, r=fee1-dead,Urgau

Strip frontmatter in fewer places

* Stop stripping frontmatter in `proc_macro::Literal::from_str` (RUST-146132)
* Stop stripping frontmatter in expr-ctxt (but not item-ctxt!) `include`s (RUST-145945)
* Stop stripping shebang (!) in `proc_macro::Literal::from_str`
  * Not a breaking change because it did compare spans already to ensure there wasn't extra whitespace or comments (`Literal::from_str("#!\n0")` already yields `Err(_)` thankfully!)
* Stop stripping frontmatter+shebang inside some rustdoc code where it doesn't make any observable difference (see self review comments)
* (Stop stripping frontmatter+shebang inside internal test code)

Fixes #145945.
Fixes #146132.

r? fee1-dead
@fmease fmease deleted the frontmatter-containment branch September 11, 2025 00:44
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 18, 2025
…fee1-dead,Urgau

Strip frontmatter in fewer places

* Stop stripping frontmatter in `proc_macro::Literal::from_str` (RUST-146132)
* Stop stripping frontmatter in expr-ctxt (but not item-ctxt!) `include`s (RUST-145945)
* Stop stripping shebang (!) in `proc_macro::Literal::from_str`
  * Not a breaking change because it did compare spans already to ensure there wasn't extra whitespace or comments (`Literal::from_str("#!\n0")` already yields `Err(_)` thankfully!)
* Stop stripping frontmatter+shebang inside some rustdoc code where it doesn't make any observable difference (see self review comments)
* (Stop stripping frontmatter+shebang inside internal test code)

Fixes rust-lang#145945.
Fixes rust-lang#146132.

r? fee1-dead
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. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue.
Projects
None yet
6 participants