-
-
Notifications
You must be signed in to change notification settings - Fork 801
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
Make compiler hint to import unqualified types/values #4304
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -392,6 +392,8 @@ impl Environment<'_> { | |
.ok_or_else(|| UnknownTypeConstructorError::Type { | ||
name: name.clone(), | ||
hint: self.unknown_type_hint(name), | ||
suggestions: self | ||
.suggest_modules_for_type_or_value(Imported::Type(name.clone())), | ||
}), | ||
|
||
Some((module_name, _)) => { | ||
|
@@ -440,6 +442,8 @@ impl Environment<'_> { | |
UnknownTypeConstructorError::Type { | ||
name: name.clone(), | ||
hint: self.unknown_type_hint(name), | ||
suggestions: self | ||
.suggest_modules_for_type_or_value(Imported::Type(name.clone())), | ||
} | ||
}), | ||
|
||
|
@@ -476,6 +480,8 @@ impl Environment<'_> { | |
name: name.clone(), | ||
variables: self.local_value_names(), | ||
type_with_name_in_scope, | ||
suggestions: self | ||
.suggest_modules_for_type_or_value(Imported::Value(name.clone())), | ||
} | ||
}), | ||
|
||
|
@@ -741,6 +747,50 @@ impl Environment<'_> { | |
.collect() | ||
} | ||
|
||
/// Suggest modules to import or use, for an unqualified type | ||
pub fn suggest_modules_for_type_or_value( | ||
&self, | ||
imported: Imported, | ||
) -> Vec<TypeOrVariableSuggestion> { | ||
let mut suggestions = self | ||
.importable_modules | ||
.iter() | ||
.filter_map(|(importable, module_info)| { | ||
match &imported { | ||
// Don't suggest importing modules if they are already imported | ||
_ if self | ||
.imported_modules | ||
.contains_key(importable.split('/').last().unwrap_or(importable)) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no fixed relationship between the name used once imported and the name of a module, and this code will produce the same name for multiple modules, so this doesn't successfully skip over already imported modules. |
||
{ | ||
None | ||
} | ||
Imported::Type(name) if module_info.get_public_type(name).is_some() => { | ||
Some(TypeOrVariableSuggestion::Importable(importable.clone())) | ||
} | ||
Imported::Value(name) if module_info.get_public_value(name).is_some() => { | ||
Some(TypeOrVariableSuggestion::Importable(importable.clone())) | ||
} | ||
_ => None, | ||
} | ||
}) | ||
.collect_vec(); | ||
|
||
suggestions.extend(self.imported_modules.iter().filter_map( | ||
|(module, (_, module_info))| match &imported { | ||
Imported::Type(name) if module_info.get_public_type(name).is_some() => { | ||
Some(TypeOrVariableSuggestion::Imported(module.clone())) | ||
} | ||
Imported::Value(name) if module_info.get_public_value(name).is_some() => { | ||
Some(TypeOrVariableSuggestion::Imported(module.clone())) | ||
} | ||
_ => None, | ||
}, | ||
)); | ||
|
||
// Filter and sort options based on if its already imported and on alphabetical order. | ||
suggestions.into_iter().sorted().collect() | ||
} | ||
|
||
/// Suggest modules to import or use, for an unknown module | ||
pub fn suggest_modules(&self, module: &str, imported: Imported) -> Vec<ModuleSuggestion> { | ||
let mut suggestions = self | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,38 @@ pub enum UnknownField { | |
NoFields, | ||
} | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] | ||
pub enum TypeOrVariableSuggestion { | ||
Imported(EcoString), | ||
Importable(EcoString), | ||
} | ||
|
||
impl TypeOrVariableSuggestion { | ||
pub fn suggestion(&self, name: &str, is_type: bool) -> String { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never use bools as arguments please 🙏 This could be a |
||
let module = self.name(); | ||
|
||
if is_type { | ||
format!("Did you mean to import `{module}.{{type {name}}}`?") | ||
} else { | ||
format!("Did you mean to import `{module}.{{{name}}}`") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to encourage unqualified importing of values. Perhaps we should only make these suggestions for types and record constructors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay! Tying with my point below, we could write the "generic" value case as "Did you mean to reference |
||
} | ||
} | ||
|
||
pub fn name(&self) -> &EcoString { | ||
match self { | ||
TypeOrVariableSuggestion::Imported(name) | ||
| TypeOrVariableSuggestion::Importable(name) => name, | ||
} | ||
} | ||
|
||
pub fn last_name_component(&self) -> &str { | ||
match self { | ||
TypeOrVariableSuggestion::Imported(name) | ||
| TypeOrVariableSuggestion::Importable(name) => name.split('/').last().unwrap_or(name), | ||
} | ||
} | ||
} | ||
|
||
/// A suggestion for an unknown module | ||
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] | ||
pub enum ModuleSuggestion { | ||
|
@@ -173,12 +205,14 @@ pub enum Error { | |
name: EcoString, | ||
variables: Vec<EcoString>, | ||
type_with_name_in_scope: bool, | ||
suggestions: Vec<TypeOrVariableSuggestion>, | ||
}, | ||
|
||
UnknownType { | ||
location: SrcSpan, | ||
name: EcoString, | ||
hint: UnknownTypeHint, | ||
suggestions: Vec<TypeOrVariableSuggestion>, | ||
}, | ||
|
||
UnknownModule { | ||
|
@@ -1153,6 +1187,7 @@ pub enum UnknownValueConstructorError { | |
name: EcoString, | ||
variables: Vec<EcoString>, | ||
type_with_name_in_scope: bool, | ||
suggestions: Vec<TypeOrVariableSuggestion>, | ||
}, | ||
|
||
Module { | ||
|
@@ -1178,11 +1213,13 @@ pub fn convert_get_value_constructor_error( | |
name, | ||
variables, | ||
type_with_name_in_scope, | ||
suggestions, | ||
} => Error::UnknownVariable { | ||
location, | ||
name, | ||
variables, | ||
type_with_name_in_scope, | ||
suggestions, | ||
}, | ||
|
||
UnknownValueConstructorError::Module { name, suggestions } => Error::UnknownModule { | ||
|
@@ -1236,6 +1273,7 @@ pub enum UnknownTypeConstructorError { | |
Type { | ||
name: EcoString, | ||
hint: UnknownTypeHint, | ||
suggestions: Vec<TypeOrVariableSuggestion>, | ||
}, | ||
|
||
Module { | ||
|
@@ -1257,10 +1295,15 @@ pub fn convert_get_type_constructor_error( | |
module_location: Option<SrcSpan>, | ||
) -> Error { | ||
match e { | ||
UnknownTypeConstructorError::Type { name, hint } => Error::UnknownType { | ||
UnknownTypeConstructorError::Type { | ||
name, | ||
hint, | ||
suggestions, | ||
} => Error::UnknownType { | ||
location: *location, | ||
name, | ||
hint, | ||
suggestions, | ||
}, | ||
|
||
UnknownTypeConstructorError::Module { name, suggestions } => Error::UnknownModule { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,3 +35,4 @@ error: Unknown variable | |
│ ^^^ | ||
|
||
The name `zoo` is not in scope here. | ||
Hint: Did you mean to import `wibble.{zoo}` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,3 +23,4 @@ error: Unknown type | |
The type `Wibble` is not defined or imported in this module. | ||
There is a value in scope with the name `Wibble`, but no type in scope with | ||
that name. | ||
Hint: Did you mean to import `module.{type Wibble}`? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the ` should be before the import keyword? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we make the "import" part of the "code" section (between backticks), it should probably be written "Did you mean to write There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, the current code for module suggestions (in the qualified case) says "Did you mean to import |
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.
There's a vector of suggestions, but only the first one is used. Seems like only one should be given in that case.