libsyntax/parse/parser.rs became too big for GitHub
#60015
Comments
petrochenkov
added
A-diagnostics
|
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 Aside: |
|
@petrochenkov I'd like to work on this issue 😃 I might need some guidance as I'm a beginner to Rust and |
|
@agnxy |
|
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. |
Add a tidy check for files with over 3,000 lines Files with a large number of lines can cause issues in GitHub (e.g. #60015) and also tend to be indicative of opportunities to refactor into less monolithic structures. This adds a new check to tidy to warn against files that have more than 3,000 lines, as suggested in #60015 (comment). (This number was chosen fairly arbitrarily as a reasonable indicator of size.) This check can be ignored with `// ignore-tidy-filelength`. Existing files with greater than 3,000 lines currently ignore the check, but this helps us spot when files are getting too large. (We might try to split up all files larger than this in the future, as in #60015).
move some functions from parser.rs to diagostics.rs Starting with a few functions mentioned in rust-lang#60015 (comment). We might refactor parser.rs further in subsequent changes. r? @petrochenkov
move some functions from parser.rs to diagostics.rs Starting with a few functions mentioned in rust-lang#60015 (comment). We might refactor parser.rs further in subsequent changes. r? @petrochenkov
|
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 |
|
Thanks @petrochenkov . I'll try to figure out more items for |
|
@petrochenkov , shall I also move |
Tweak `self` arg not as first argument of a method diagnostic
Mention that `self` is only valid on "associated functions"
```
error: unexpected `self` argument in function
--> $DIR/self-in-function-arg.rs:1:15
|
LL | fn foo(x:i32, self: i32) -> i32 { self }
| ^^^^ not valid as function argument
|
= note: `self` is only valid as the first argument of an associated function
```
When it is a method, mention it must be first
```
error: unexpected `self` argument in function
--> $DIR/trait-fn.rs:4:20
|
LL | fn c(foo: u32, self) {}
| ^^^^ must be the first associated function argument
```
Move a bunch of error recovery methods to `diagnostics.rs` away from `parser.rs`.
Fix rust-lang#51547. CC rust-lang#60015.
Tweak `self` arg not as first argument of a method diagnostic
Mention that `self` is only valid on "associated functions"
```
error: unexpected `self` argument in function
--> $DIR/self-in-function-arg.rs:1:15
|
LL | fn foo(x:i32, self: i32) -> i32 { self }
| ^^^^ not valid as function argument
|
= note: `self` is only valid as the first argument of an associated function
```
When it is a method, mention it must be first
```
error: unexpected `self` argument in function
--> $DIR/trait-fn.rs:4:20
|
LL | fn c(foo: u32, self) {}
| ^^^^ must be the first associated function argument
```
Move a bunch of error recovery methods to `diagnostics.rs` away from `parser.rs`.
Fix rust-lang#51547. CC rust-lang#60015.
|
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. |
libsyntax: Refactor `parser.rs` into reasonably sized logical units
Here we split `parser.rs` (~7.9 KLOC) into more reasonably sized files (all < 1.8 KLOC):
- `./src/libsyntax/parse/`
- `parser.rs`
- `parser/`
- `pat.rs`
- `expr.rs`
- `stmt.rs`
- `ty.rs`
- `path.rs`
- `generics.rs`
- `item.rs`
- `module.rs`
Closes #60015.
r? @petrochenkov
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.The text was updated successfully, but these errors were encountered: