Open Centri3 opened 1 year ago
Rajveer100
commented
1 year ago @Centri3 Here's some brief info about me, having a conversation the first time.
How do we target the rustc command for the compiler with [options] for specifying [file name] rather than the one installed in the system's bin? Once that's done I can test changes to be reflected and move forward.
[PS: Issue was resolved via zulip]
Rajveer100
commented
1 year ago @rustbot claim
chenyukang
commented
1 year ago @Rajveer100 welcome! I used this for run rustc compiled:
rustup toolchain link dev ./build/aarch64-apple-darwin/stage1/
RUST_BACKTRACE=1 rustc +dev {{FILE}}If you use justfile, this is my frequently used commands for rustc, FYI.
https://gist.github.com/chenyukang/1483cbbf75a4bd5ae2930415329cb682
Rajveer100
commented
1 year ago @chenyukang Thanks for the info! Looks pretty good!
Rajveer100
commented
1 year ago Backtrace for the issue:
estebank
commented
1 year ago Be aware that this can't be handled in the parser because this is syntactically supported.
Rajveer100
commented
10 months ago @estebank
The issue seems pretty clear to me, we just want to improve the diagnostics when passing named arguments to a function instead of just saying () which is a unit type.
Could you let me know the target files/dir which handles such logic so I can accordingly work on it?
Also, as an improvement, we could also consider named arguments for this form <name_of_argument>: <argument> too, as certain languages use this rather than =.
Let me know what you think.
PS: Just noticed, the : part already has an issue mentioned with an ongoing PR.
estebank
commented
10 months ago @Rajveer100 this will actually be slightly difficult to handle (due to the case I mentioned in my previous comment). What you'd have to do is modify the parser to detect this (effectively, track that you've started to parse a function or method call, that you're directly parsing an expression that's meant to be an argument), keep that data around in a side-channel, some ParseSess diagnostic field with the span of these, and then during resolve and typeck you need to look at that information to silence the errors that happen now and emit a single error about named parameters not being supported. I am not able to provide pointers for all of these at the moment, but you can start looking at the parse_expr_fn_call and parse_dot_or_call_expr methods for the parser part of it, use -Ztrack-diagnostics to have all diagnostics produce a note pointing which file the error is being created at and -Z=treat-err-as-bug=1 to have the compiler ICE when it encounters the first error (you can change the 1 to be any number and it will skip that many before ICEing) to get a backtrace of where the error is being emitted.
Rajveer100
commented
10 months ago $ rustc debug.rs -Ztrack-diagnostics -Ztreat-err-as-bug=1error[E0425]: cannot find value `x` in this scope
--> debug.rs:12:7
|
1 | fn a(x: u32, y: u32) {}
| -------------------- similarly named function `a` defined here
...
12 | a(x = x_2, y = y_2);
| ^ help: a function with a similar name exists: `a`
-Ztrack-diagnostics: created at compiler/rustc_resolve/src/late/diagnostics.rs:428:39
```
thread 'rustc' panicked at compiler/rustc_errors/src/lib.rs:1738:30:
aborting due to `-Z treat-err-as-bug=1`
stack backtrace:
0: 0x101b90268 -
Rajveer100
commented
10 months ago What you'd have to do is modify the parser to detect this (effectively, track that you've started to parse a function or method call, that you're directly parsing an expression that's meant to be an argument), keep that data around in a side-channel, some
ParseSessdiagnostic field with the span of these, and then during resolve and typeck you need to look at that information to silence the errors that happen now and emit a single error about named parameters not being supported.
You mean to say we need to add an additional field in the Parser struct with ParseSess as datatype, which we can use to compare the last and current argument, where if we find such matching =, emit the new diagnostic?
Probably this place:
#[derive(Clone)]
pub struct Parser<'a> {
pub sess: &'a ParseSess,
/// The current token.
pub token: Token,
/// The spacing for the current token.
pub token_spacing: Spacing,
/// The previous token.
pub prev_token: Token,
pub capture_cfg: bool,
restrictions: Restrictions,
expected_tokens: Vec<TokenType>,
token_cursor: TokenCursor,
...Yeah, you could add it to Parser, but we try to avoid adding that kind of "global" state as much as possible. Normally we'd add a field to the AST, but because this is exclusively for error recover (so we don't want to add any performance penalty when not hit) and this will be very uncommon, it seems like the best option available to us.
Rajveer100
commented
9 months ago Could you describe how we would store the data using ParseSess?
I see this, do we need an additional field here as well and what type would it have, probably bool?:
pub(crate) struct ParseSess {
parse_sess: RawParseSess,
ignore_path_set: Lrc<IgnorePathSet>,
can_reset_errors: Lrc<AtomicBool>,
}@estebank
Under parse_expr_fn_call:
let open_paren = self.token.span;Is this essentially the tokenizer's eat method that we use in parsing for getting rid of the first open ( in the function definition?
Which token would be checked here for the function parameter, so I can store it, also once that's done, where is the resolve and diagnostic done do I can retrieve my side-channel data and throw a single error?
estebank
commented
5 months ago Is this essentially the tokenizer's
eatmethod that we use in parsing for getting rid of the first open(in the function definition?
No, parse_paren_comma_seq which gets called within parse_expr_paren_seq takes care of the opening and closing parens.
I think this can be done entirely in rustc_resolve, though. We already emit a suggestion to introduce a let binding, that we can modify to look to see if the parent of the assign expression is an fn or method call, and if so instead of suggesting let, suggest deleting the name and = signs with an explanation that we don't have named arguments.
Rajveer100
commented
5 months ago Yeah, I have found the locations for this, for the parent part, do we do something like this:
if let Some(Expr { _: ExprKind::MethodCall(_, ..), .. }) = self.parent <--?? {Also, the name field for this issue would be x = x where we then strip to just x and throw a span suggestion?
Code
Current output
Desired output
Rationale and extra context
For somebody new to rust, it may not be immediately clear why this doesn't work, or why it's
(). We'd ideally guide them in the right direction if lhs' name is the same as that parameter.I think we only need to do this for the two above cases (where lhs doesn't exist or it does, but the type is wrong)
If the correct type happens to be
(), this could probably be a clippy lint.Other cases
No response
Anything else?
No response