`rustc_parse` top-level cleanups #125815

nnethercote · 2024-05-31T12:20:08Z

A bunch of improvements in and around compiler/rustc_parse/src/lib.rs. Many of the changes streamline the API in that file from this (12 functions and one macro):

    name                              args                  return type
    ----                              ----                  -----------
    panictry_buffer!                  Result<T, Vec<Diag>>  T

pub parse_crate_from_file             path                  PResult<Crate>
pub parse_crate_attrs_from_file       path                  PResult<AttrVec>
pub parse_crate_from_source_str       name,src              PResult<Crate>
pub parse_crate_attrs_from_source_str name,src              PResult<AttrVec>

pub new_parser_from_source_str        name,src              Parser
pub maybe_new_parser_from_source_str  name,src              Result<Parser, Vec<Diag>>
pub new_parser_from_file              path,error_sp         Parser
    maybe_source_file_to_parser       srcfile               Result<Parser, Vec<Diag>>

pub parse_stream_from_source_str      name,src,override_sp  TokenStream
pub source_file_to_stream             srcfile,override_sp   TokenStream
    maybe_file_to_stream              srcfile,override_sp   Result<TokenStream, Vec<Diag>>

pub stream_to_parser                  stream,subparser_name Parser

to this:

    name                              args                  return type
    ----                              ----                  -----------
    unwrap_or_emit_fatal              Result<T, Vec<Diag>>  T
  
pub new_parser_from_source_str        name,src              Result<Parser, Vec<Diag>>
pub new_parser_from_file              path,error_sp         Result<Parser, Vec<Diag>>
    new_parser_from_source_file       srcfile               Result<Parser, Vec<Diag>>

pub source_str_to_stream              name,src,override_sp  Result<TokenStream, Vec<Diag>>
    source_file_to_stream             srcfile,override_sp   Result<TokenStream, Vec<Diag>>

I found the old API quite confusing, with lots of similar-sounding function names and no clear structure. I think the new API is much better.

r? @spastorino

rustbot · 2024-05-31T12:20:17Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

nnethercote · 2024-05-31T12:20:27Z

Best reviewed one commit at a time. The commit messages explain each commit.

ytmimi · 2024-05-31T13:07:14Z

src/tools/rustfmt/src/parse/parser.rs

-                new_parser_from_file(psess, file, None)
+                unwrap_or_emit_fatal(new_parser_from_file(psess, file, None))


Did new_parser_from_file used to emit a fatal diagnostic? I Haven't looked at any of the changes outside of src/tools/rustfmt, and just want to make sure we're not changing existing behavior.

Correct. unwrap_or_emit_fatal(new_parser_from_file(...)) is equivalent to the old new_parser_from_file(...). So there is no change to existing behaviour.

A nice thing about this is that the catch_unwind will be removable in a follow-up, removing the asymmetry between the Input::File and Input::Text cases here.

That's great! I appreciate the explanation.

I've had time to do the follow-up, which I added as an extra commit to this PR.

Awesome! Thanks for the updated :)

The `Input::File` and `Input::Text` cases should be very similar. However, currently the `Input::File` case uses `catch_unwind` because, until recently (rust-lang#125815) there was a fallible version of `new_parser_from_source_str` but only an infallible version of `new_parser_from_file`. This difference wasn't fundamental, just an overlooked gap in the API of `rustc_parse`. Both of those operations are now fallible, so the `Input::File` and `Input::Text` cases can made more similar, with no need for `catch_unwind`. This also lets us simplify an `Option<Vec<Diag>>` to `Vec<Diag>`.

spastorino · 2024-06-04T19:24:10Z

compiler/rustc_parse/src/lib.rs

@@ -34,11 +34,6 @@ mod errors;

 rustc_fluent_macro::fluent_messages! { "../messages.ftl" }

-// A bunch of utility functions of the form `parse_<thing>_from_<source>`
-// where <thing> includes crate, expr, item, stmt, tts, and one that
-// uses a HOF to parse anything, and <source> includes file and


And I don't know what a "HOF" is.

Higher Order Function. Anyway ...

spastorino · 2024-06-04T19:27:05Z

@bors r+ rollup

bors · 2024-06-04T19:27:08Z

📌 Commit 3feebde has been approved by spastorino

It is now in the queue for this repository.

…llaumeGomez Rollup of 11 pull requests Successful merges: - rust-lang#106186 (Add function `core::iter::chain`) - rust-lang#125596 (Convert `proc_macro_back_compat` lint to an unconditional error.) - rust-lang#125696 (Explain differences between `{Once,Lazy}{Cell,Lock}` types) - rust-lang#125917 (Create `run-make` `env_var` and `env_var_os` helpers) - rust-lang#125927 (Ignore `vec_deque_alloc_error::test_shrink_to_unwind` test on non-unwind targets) - rust-lang#125930 (feat(opt-dist): new flag `--benchmark-cargo-config`) - rust-lang#125932 (Fix typo in the docs of `HashMap::raw_entry_mut`) - rust-lang#125933 (Update books) - rust-lang#125944 (Update fuchsia maintainers) - rust-lang#125946 (Include trailing commas in wrapped function declarations [RustDoc]) - rust-lang#125973 (Remove `tests/run-make-fulldeps/pretty-expanded`) Failed merges: - rust-lang#125815 (`rustc_parse` top-level cleanups) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-06-04T23:41:41Z

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

- Avoid unnecessary escaping of single quotes within string literals. - Add a missing blank line between two `UNICODE_ARRAY` sections.

Lexing converts source text into a token stream. Parsing converts a token stream into AST fragments. This commit renames several lexing operations that have "parse" in the name. I think these names have been subtly confusing me for years. This is just a `s/parse/lex/` on function names, with one exception: `parse_stream_from_source_str` becomes `source_str_to_stream`, to make it consistent with the existing `source_file_to_stream`. The commit also moves that function's location in the file to be just above `source_file_to_stream`. The commit also cleans up a few comments along the way.

It has a single call site. This also means `CFG_ATTR_{GRAMMAR_HELP,NOTE_REF}` can be moved into `parse_cfg_attr`, now that it's the only function that uses them. And the commit removes the line break in the URL.

Because it takes an `Lrc<SourceFile>`, and for consistency with `source_file_to_stream`.

It's a zero-value wrapper of `Parser::new`.

- Convert it from a macro to a function, which is nicer. - Rename it as `unwrap_or_emit_fatal`, which is clearer. - Fix the comment. In particular, `panictry!` no longer exists. - Remove the unnecessary `use` declaration.

The first one is out-of-date -- there are no longer functions expr, item, stmt. And I don't know what a "HOF" is. The second one doesn't really tell you anything.

…_file`. For consistency with `new_parser_from_{file,source_str}` and `maybe_new_parser_from_source_str`.

It does exactly what is required.

All four functions are simple and have a single call site. This requires making `Parser::parse_inner_attributes` public, which is no big deal.

It's the only one of these functions where `psess` isn't the first argument.

It has a single call site.

Currently we have an awkward mix of fallible and infallible functions: ``` new_parser_from_source_str maybe_new_parser_from_source_str new_parser_from_file (maybe_new_parser_from_file) // missing (new_parser_from_source_file) // missing maybe_new_parser_from_source_file source_str_to_stream maybe_source_file_to_stream ``` We could add the two missing functions, but instead this commit removes of all the infallible ones and renames the fallible ones leaving us with these which are all fallible: ``` new_parser_from_source_str new_parser_from_file new_parser_from_source_file source_str_to_stream source_file_to_stream ``` This requires making `unwrap_or_emit_fatal` public so callers of formerly infallible functions can still work. This does make some of the call sites slightly more verbose, but I think it's worth it for the simpler API. Also, there are two `catch_unwind` calls and one `catch_fatal_errors` call in this diff that become removable thanks this change. (I will do that in a follow-up PR.)

The `Input::File` and `Input::Text` cases should be very similar. However, currently the `Input::File` case uses `catch_unwind` because, until recently (rust-lang#125815) there was a fallible version of `new_parser_from_source_str` but only an infallible version of `new_parser_from_file`. This difference wasn't fundamental, just an overlooked gap in the API of `rustc_parse`. Both of those operations are now fallible, so the `Input::File` and `Input::Text` cases can made more similar, with no need for `catch_unwind`. This also lets us simplify an `Option<Vec<Diag>>` to `Vec<Diag>`.

nnethercote · 2024-06-05T02:45:17Z

I rebased.

@bors r=spastorino

bors · 2024-06-05T02:45:19Z

📌 Commit 2d4e7df has been approved by spastorino

It is now in the queue for this repository.

…kingjubilee Rollup of 12 pull requests Successful merges: - rust-lang#123168 (Add `size_of` and `size_of_val` and `align_of` and `align_of_val` to the prelude) - rust-lang#125273 (bootstrap: implement new feature `bootstrap-self-test`) - rust-lang#125683 (Rewrite `suspicious-library`, `resolve-rename` and `incr-prev-body-beyond-eof` `run-make` tests in `rmake.rs` format) - rust-lang#125815 (`rustc_parse` top-level cleanups) - rust-lang#125903 (rustc_span: Inline some hot functions) - rust-lang#125906 (Remove a bunch of redundant args from `report_method_error`) - rust-lang#125920 (Allow static mut definitions with #[linkage]) - rust-lang#125982 (Make deleting on LinkedList aware of the allocator) - rust-lang#125995 (Use inline const blocks to create arrays of `MaybeUninit`.) - rust-lang#125996 (Closures are recursively reachable) - rust-lang#126003 (Add a co-maintainer for the two ARMv4T targets) - rust-lang#126004 (Add another test for hidden types capturing lifetimes that outlive but arent mentioned in substs) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#125815 - nnethercote:rustc_parse-top-level-cleanups, r=spastorino `rustc_parse` top-level cleanups A bunch of improvements in and around `compiler/rustc_parse/src/lib.rs`. Many of the changes streamline the API in that file from this (12 functions and one macro): ``` name args return type ---- ---- ----------- panictry_buffer! Result<T, Vec<Diag>> T pub parse_crate_from_file path PResult<Crate> pub parse_crate_attrs_from_file path PResult<AttrVec> pub parse_crate_from_source_str name,src PResult<Crate> pub parse_crate_attrs_from_source_str name,src PResult<AttrVec> pub new_parser_from_source_str name,src Parser pub maybe_new_parser_from_source_str name,src Result<Parser, Vec<Diag>> pub new_parser_from_file path,error_sp Parser maybe_source_file_to_parser srcfile Result<Parser, Vec<Diag>> pub parse_stream_from_source_str name,src,override_sp TokenStream pub source_file_to_stream srcfile,override_sp TokenStream maybe_file_to_stream srcfile,override_sp Result<TokenStream, Vec<Diag>> pub stream_to_parser stream,subparser_name Parser ``` to this: ``` name args return type ---- ---- ----------- unwrap_or_emit_fatal Result<T, Vec<Diag>> T pub new_parser_from_source_str name,src Result<Parser, Vec<Diag>> pub new_parser_from_file path,error_sp Result<Parser, Vec<Diag>> new_parser_from_source_file srcfile Result<Parser, Vec<Diag>> pub source_str_to_stream name,src,override_sp Result<TokenStream, Vec<Diag>> source_file_to_stream srcfile,override_sp Result<TokenStream, Vec<Diag>> ``` I found the old API quite confusing, with lots of similar-sounding function names and no clear structure. I think the new API is much better. r? `@spastorino`

…ups, r=spastorino More `rustc_parse` cleanups Following on from rust-lang#125815. r? `@spastorino`

Rollup merge of rust-lang#126052 - nnethercote:rustc_parse-more-cleanups, r=spastorino More `rustc_parse` cleanups Following on from rust-lang#125815. r? `@spastorino`

The `Input::File` and `Input::Text` cases should be very similar. However, currently the `Input::File` case uses `catch_unwind` because, until recently (rust-lang#125815) there was a fallible version of `new_parser_from_source_str` but only an infallible version of `new_parser_from_file`. This difference wasn't fundamental, just an overlooked gap in the API of `rustc_parse`. Both of those operations are now fallible, so the `Input::File` and `Input::Text` cases can made more similar, with no need for `catch_unwind`. This also lets us simplify an `Option<Vec<Diag>>` to `Vec<Diag>`.

rustc's high-level lexer now provides a public interface returning a TokenStream, so we now use that rather than making a Parser and pulling tokens from it one by one. (And in any case the previous approach no longer works, because Parser::token_spacing is no longer public.) See rust-lang/rust#125815 rust-lang/rust#126052 Other rustc changes: ParseSess now provides a dcx() method rather than a public dcx field. There are new NtIdent and NtLifetime TokenKinds, which AIUI won't appear in token streams created by the lexer.

rustbot assigned spastorino May 31, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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. labels May 31, 2024

This comment has been minimized.

# to view

nnethercote force-pushed the rustc_parse-top-level-cleanups branch from f3eb8ce to b7a73c6 Compare May 31, 2024 12:25

ytmimi reviewed May 31, 2024

View reviewed changes

nnethercote force-pushed the rustc_parse-top-level-cleanups branch from b7a73c6 to 3feebde Compare June 4, 2024 00:01

spastorino reviewed Jun 4, 2024

View reviewed changes

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 Jun 4, 2024

GuillaumeGomez mentioned this pull request Jun 4, 2024

Rollup of 11 pull requests #125989

Merged

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 Jun 4, 2024

nnethercote added 9 commits June 5, 2024 10:29

UNICODE_ARRAY and ASCII_ARRAY fixes.

e1ae0fa

- Avoid unnecessary escaping of single quotes within string literals. - Add a missing blank line between two `UNICODE_ARRAY` sections.

Inline and remove error_malformed_cfg_attr_missing.

f657624

It has a single call site. This also means `CFG_ATTR_{GRAMMAR_HELP,NOTE_REF}` can be moved into `parse_cfg_attr`, now that it's the only function that uses them. And the commit removes the line break in the URL.

Rename maybe_file_to_stream as maybe_source_file_to_stream.

769ca3f

Because it takes an `Lrc<SourceFile>`, and for consistency with `source_file_to_stream`.

Remove stream_to_parser.

3c321b9

It's a zero-value wrapper of `Parser::new`.

Improve panictry_buffer!.

af13b48

- Convert it from a macro to a function, which is nicer. - Rename it as `unwrap_or_emit_fatal`, which is clearer. - Fix the comment. In particular, `panictry!` no longer exists. - Remove the unnecessary `use` declaration.

Remove low-value comments.

29e6e28

The first one is out-of-date -- there are no longer functions expr, item, stmt. And I don't know what a "HOF" is. The second one doesn't really tell you anything.

Rename maybe_source_file_to_parser as `maybe_new_parser_from_source…

191b76e

…_file`. For consistency with `new_parser_from_{file,source_str}` and `maybe_new_parser_from_source_str`.

Use source_str_to_stream in a test file.

8964106

It does exactly what is required.

nnethercote added 5 commits June 5, 2024 10:38

Inline and remove parse_crate{,_attrs}_from_{file,source_str}.

25972ae

All four functions are simple and have a single call site. This requires making `Parser::parse_inner_attributes` public, which is no big deal.

Reorder source_str_to_stream arguments.

ab192a0

It's the only one of these functions where `psess` isn't the first argument.

Inline and remove source_file_to_stream.

264dbe4

It has a single call site.

nnethercote force-pushed the rustc_parse-top-level-cleanups branch from 3feebde to 2d4e7df Compare June 5, 2024 02:44

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 Jun 5, 2024

workingjubilee mentioned this pull request Jun 5, 2024

Rollup of 12 pull requests #126016

Merged

bors merged commit 05b4674 into rust-lang:master Jun 5, 2024
6 checks passed

rustbot added this to the 1.80.0 milestone Jun 5, 2024

nnethercote deleted the rustc_parse-top-level-cleanups branch June 5, 2024 22:24

nnethercote mentioned this pull request Jun 5, 2024

More rustc_parse cleanups #126052

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 7, 2024

Rollup merge of rust-lang#126052 - nnethercote:rustc_parse-more-clean…

4aceaaa

…ups, r=spastorino More `rustc_parse` cleanups Following on from rust-lang#125815. r? `@spastorino`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustc_parse` top-level cleanups #125815

`rustc_parse` top-level cleanups #125815

nnethercote commented May 31, 2024

rustbot commented May 31, 2024

nnethercote commented May 31, 2024

This comment has been minimized.

ytmimi May 31, 2024

nnethercote May 31, 2024

ytmimi May 31, 2024

nnethercote Jun 4, 2024

ytmimi Jun 4, 2024

spastorino Jun 4, 2024

spastorino commented Jun 4, 2024

bors commented Jun 4, 2024

bors commented Jun 4, 2024

nnethercote commented Jun 5, 2024

bors commented Jun 5, 2024

		new_parser_from_file(psess, file, None)
		unwrap_or_emit_fatal(new_parser_from_file(psess, file, None))

rustc_parse top-level cleanups #125815

rustc_parse top-level cleanups #125815

Conversation

nnethercote commented May 31, 2024

rustbot commented May 31, 2024

nnethercote commented May 31, 2024

This comment has been minimized.

ytmimi May 31, 2024

Choose a reason for hiding this comment

nnethercote May 31, 2024

Choose a reason for hiding this comment

ytmimi May 31, 2024

Choose a reason for hiding this comment

nnethercote Jun 4, 2024

Choose a reason for hiding this comment

ytmimi Jun 4, 2024

Choose a reason for hiding this comment

spastorino Jun 4, 2024

Choose a reason for hiding this comment

spastorino commented Jun 4, 2024

bors commented Jun 4, 2024

bors commented Jun 4, 2024

nnethercote commented Jun 5, 2024

bors commented Jun 5, 2024

`rustc_parse` top-level cleanups #125815

`rustc_parse` top-level cleanups #125815