-
-
Notifications
You must be signed in to change notification settings - Fork 771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: check if module have type with the same name #3696
Conversation
I don't think the suggestion is right here: "Did you mean type X" would produce invalid code outside of imports, the rest of the error message looks good to me though :) |
This is what I thought! maybe we need to differentiate when we are importing or accessing it🤔 |
Yeah I think that's the right call |
When you are ready for a review please un-draft this PR. Thank you! |
cf6aa60
to
f1c3971
Compare
Hello! Are you still working on this one? Thanks |
f1c3971
to
5ca6c27
Compare
Hi! sorry for kinda forget about this one, as discussed with @giacomocavalieri I replace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I've left some comments inline.
compiler-core/src/error.rs
Outdated
format!("The module `{module_name}` does not have a `{name}` value.") | ||
let text = match context { | ||
ModuleValueErrorContext::Import { type_with_same_name: true } => | ||
format!("`{name}` is only a type, it cannot be imported as a value."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we have it show how to import it as a type, using the type
keyword?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that we already have the diagnostic to hint users to use the type
keyword, do we still want to do it?
bed355d
to
d8b7807
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work again! Thank you Frank!!
resolve: #3691
the compiler already suggests something like:
do we want to change it?