Rust: `libsyntax/parse/parser.rs` became too big for GitHub

Created on 16 Apr 2019  路  15Comments  路  Source: rust-lang/rust

Very convenient "Blame" functionality doesn't work in particular.

The solution is to move something from that file.
The obvious candidate for moving is diagnostics-related code, that can be moved into libsyntax/parse/diagnostics.rs.

A-diagnostics A-frontend E-easy P-high

Most helpful comment

Heh, if I am not mistaking, parser.rs is the largest non-generated Rust file out there :) I use it for benchmarking my parsers and will be sad to see it go /s

On a more serious note, I quite like the grammar layout I use for rust-analyzer.

There's also a vague desire to make a parsing library, which will work inside rustc, syn and IDE. I don't think we should actively plan for this right now, but it might be a good idea to keep in mind. Specifically, it might be interesting to take a look at how swift's parser is organized: IIUC, it produces both the traditional AST, like rustc, and a Swift libsyntanx concrete syntax tree. This seems like a setup we might want to do as well, it is consistent with current RLS-2.0 approach.

All 15 comments

After the diagnostics extraction, ...

...I think we should split the parser up into semantic categories "items", "expressions", "types", "patterns", and so on and so forth since that makes understanding the parser much easier. E.g. you could use some Parse trait which these each of these semantic categories implement.

Aside: rustc has some ridiculously large files; we should also seriously consider making tidy warn on too large files at minimum and eventually setting some hard boundaries (e.g. > 3000 LOC => build failure) to avoid files getting so large in the future. We'd need to lower the limit incrementally of course.

@petrochenkov I'd like to work on this issue :smiley: I might need some guidance as I'm a beginner to Rust and syntax crate. I'm reading parser.rs to see what can be extracted to diagnostics.rs. Thanks.

@agnxy
Moving maybe_report_ambiguous_plus, maybe_recover_from_bad_type_plus, maybe_recover_from_bad_qpath, maybe_recover_from_bad_qpath_stage_2, maybe_consume_incorrect_semicolon, trait RecoverQPath and its impls would be a good start.

In general, if any function does only error reporting or error recovery, it can be moved.

Heh, if I am not mistaking, parser.rs is the largest non-generated Rust file out there :) I use it for benchmarking my parsers and will be sad to see it go /s

On a more serious note, I quite like the grammar layout I use for rust-analyzer.

There's also a vague desire to make a parsing library, which will work inside rustc, syn and IDE. I don't think we should actively plan for this right now, but it might be a good idea to keep in mind. Specifically, it might be interesting to take a look at how swift's parser is organized: IIUC, it produces both the traditional AST, like rustc, and a Swift libsyntanx concrete syntax tree. This seems like a setup we might want to do as well, it is consistent with current RLS-2.0 approach.

Hi,I have a noob question about the workflow. Since I may make more changes for this issue and #60348 is merged, should I create a new PR on a new branch? Thanks.

@agnxy
For already merged PRs the same branch can be reused, but it doesn't make any difference, there are no requirements on this or anything.

Thanks @petrochenkov . I'll try to figure out more items for diagnostics.rs

@petrochenkov , shall I also move recover_closing_delimiter, recover_seq_parse_error, report_invalid_macro_expansion_item, expected_item_err, err_dotdotdot_syntax and missing_assoc_item_kind_err to diagnostics.rs? I'm not sure if that's ok. Thanks a lot.

I believe this can be closed now. We should continue on the diet regardless.

Weird, when I did it a week ago it loaded fine.

It's a bit random. In the past, sometimes after getting the unicorn, hitting reload it would pop up immediately. Presumably there's some caching going on where subsequent requests finish within the time limit. In fact, I just now got it to load after reloading a few times. 馃槅

Current state: file load fine, but blame still doesn't work.

I'm fixing this permanently today.

Was this page helpful?
0 / 5 - 0 ratings