Improve MissingColonColon diagnostic. #5926
Conversation
|
This is generated here: I'm not sure that getting a function when resolving a path is actually wrong, I guess the issue is that we don't have types for functions. Code quote: |
|
I'm looking into where this is generated. Code quote: |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/diagnostic_test_data/tests line 752 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
This is generated here:
I'm not sure that getting a function when resolving a path is actually wrong, I guess the issue is that we don't have types for functions.
function will be a type of a constants if we have an Fn sort of trait. but that is not currently the case.
It makes sense to me this will change when we go though the process of adding closures.
crates/cairo-lang-semantic/src/diagnostic_test_data/tests line 762 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
I'm looking into where this is generated.
cool.
|
Previously, orizi wrote…
It is generated here: |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/diagnostic_test_data/tests line 762 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
It is generated here:
it seems that there is some "missing" propagation.
but it makes sense to me for this to go to a following PR.
|
Should I rename it to: Suggestion: MaybeMissingColonColon |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)
crates/cairo-lang-semantic/src/diagnostic.rs line 813 at r1 (raw file):
} } SemanticDiagnosticKind::MissingColonColon => "Are you missing a `::`?.".into(),
this is total nitpick from my side, but I don't like using questions as diagnostics
to given an example, this is what Rust does here:
use `::<...>` instead of `<...>` to specify lifetime, type, or const arguments
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/diagnostic.rs line 813 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
Should I rename it to:
MaybeMissingColonColon
?
Yeah, makes sense
|
Previously, mkaput (Marek Kaput) wrote…
The issue is that it is not clear what the error is. |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/diagnostic.rs line 813 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
The issue is that it is not clear what the error is.
yeah you're right
what about?
invalid path syntax, use `::<...>` instead of `<...>` to specify...
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/diagnostic.rs line 813 at r1 (raw file):
Previously, mkaput (Marek Kaput) wrote…
yeah you're right
what about?
invalid path syntax, use `::<...>` instead of `<...>` to specify...
though I do not want to block this, so we can ignore this comment
|
Previously, mkaput (Marek Kaput) wrote…
Are you confident that if you have |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/diagnostic.rs line 813 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
Are you confident that if you have
a < b
and you can't resolve 'a', then the issue is missing::?
hmm, no I cannot be certain. there would have to be a matching > to be certain I think
|
Previously, mkaput (Marek Kaput) wrote…
I think we will have to change the parser to be sure about the error here. |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/diagnostic.rs line 813 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
I think we will have to change the parser to be sure about the error here.
makes sense. ignore my comment then
ilyalesokhin-starkware
force-pushed
the
ilya/missing_diag
branch
from
37645bd
to
e2591fe
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
|
Previously, orizi wrote…
done |
This change is