Skip to content
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

[compiler-v2] Avoid infinite recursion showing types with receiver functions in presence of errors #14922

Merged
merged 3 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@

Diagnostics:
error: undeclared receiver function `borrow` for type `vector<Entry<K, V>>`
┌─ tests/checking/receiver/bad_receiver.move:25:10
25 │ &map.entries.borrow(self.index).key
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: undeclared receiver function `lower_bound` for type `OrderedMap<K, V>`
┌─ tests/checking/receiver/bad_receiver.move:29:27
29 │ let lower_bound = self.lower_bound(key);
│ ^^^^^^^^^^^^^^^^^^^^^

error: type cannot have type arguments
┌─ tests/checking/receiver/bad_receiver.move:39:36
39 │ public fun borrow<K, V>(self: &OrderedMao<K, V>, key: &K): &V {
│ ^^^^^^^^^^

error: undeclared `0x1::ordered_map::OrderedMao`
┌─ tests/checking/receiver/bad_receiver.move:39:36
39 │ public fun borrow<K, V>(self: &OrderedMao<K, V>, key: &K): &V {
│ ^^^^^^^^^^
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
module aptos_std::ordered_map {
const EITER_OUT_OF_BOUNDS: u64 = 3;

struct Entry<K, V> has drop, copy, store {
key: K,
value: V,
}

enum OrderedMap<K, V> has drop, copy, store {
SortedVectorMap {
entries: vector<Entry<K, V>>,
}
}

enum Iterator has copy, drop {
End,
Position {
index: u64,
},
}

public fun iter_borrow_key<K, V>(self: &Iterator, map: &OrderedMap<K, V>): &K {
assert!(!(self is Iterator::End), EITER_OUT_OF_BOUNDS);

&map.entries.borrow(self.index).key
}

public fun find<K, V>(self: &OrderedMap<K, V>, key: &K): Iterator {
let lower_bound = self.lower_bound(key);
if (lower_bound.iter_is_end(self)) {
lower_bound
} else if (lower_bound.iter_borrow_key(self) == key) {
lower_bound
} else {
self.new_end_iter()
}
}

public fun borrow<K, V>(self: &OrderedMao<K, V>, key: &K): &V {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be a typo in the type name OrderedMao. This should be corrected to OrderedMap to match the struct definition and maintain consistency throughout the code.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the bot correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in this case it's in a test directory, so I intended it to be incorrect, and it should not be corrected. Their bot should notice a directory named compiler.*test.* and consider the possibility that the code is test input code for a compiler.

self.find(key).iter_borrow(self)
}

public fun new_end_iter<K, V>(self: &OrderedMap<K, V>): Iterator {
Iterator::End
}

public fun iter_is_end<K, V>(self: &Iterator, _map: &OrderedMap<K, V>): bool {
self is Iterator::End
}
}
18 changes: 11 additions & 7 deletions third_party/move/move-model/src/builder/exp_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,13 +420,17 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
};
ty.visit(&mut visitor);
if incomplete {
self.error(
&loc,
&format!(
"unable to infer instantiation of type `{}` (consider providing type arguments or annotating the type)",
ty.display(&self.type_display_context())
),
);
let displayed_ty = format!("{}", ty.display(&self.type_display_context()));
// Skip displaying the error message if there is already an error in the type; we must have another message about it already.
if !displayed_ty.contains("*error*") {
self.error(
&loc,
&format!(
"unable to infer instantiation of type `{}` (consider providing type arguments or annotating the type)",
displayed_ty
),
);
}
}
ty
}
Expand Down
14 changes: 9 additions & 5 deletions third_party/move/move-model/src/builder/model_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ impl<'env> ModelBuilder<'env> {
display_type_vars: false,
used_modules: BTreeSet::new(),
use_module_qualification: false,
recursive_vars: None,
}
}

Expand Down Expand Up @@ -399,7 +400,7 @@ impl<'env> ModelBuilder<'env> {
&entry.name_loc,
&format!(
"parameter name `{}` indicates a receiver function but \
the type `{}` {}. Consider using a different name.",
the type `{}` {}. Consider using a different name.",
well_known::RECEIVER_PARAM_NAME,
base_type.display(&type_ctx()),
reason
Expand All @@ -413,7 +414,7 @@ impl<'env> ModelBuilder<'env> {
if !matches!(ty, Type::TypeParameter(_)) {
diag(&format!(
"must only use type parameters \
but instead uses `{}`",
but instead uses `{}`",
ty.display(&type_ctx())
))
} else if !seen.insert(ty) {
Expand All @@ -433,7 +434,7 @@ impl<'env> ModelBuilder<'env> {
if &entry.module_id != mid {
diag(
"is declared outside of this module \
and new receiver functions cannot be added",
and new receiver functions cannot be added",
)
} else {
// The instantiation must be fully generic.
Expand All @@ -456,7 +457,7 @@ impl<'env> ModelBuilder<'env> {
{
diag(
"is associated with the standard vector module \
and new receiver functions cannot be added",
and new receiver functions cannot be added",
)
} else {
// See above for structs
Expand All @@ -465,9 +466,12 @@ impl<'env> ModelBuilder<'env> {
.insert(name.symbol, name.clone());
}
},
Type::Error => {
// Ignore this, there will be a message where the error type is generated.
},
_ => diag(
"is not suitable for receiver functions. \
Only structs and vectors can have receiver functions",
Only structs and vectors can have receiver functions",
),
}
}
Expand Down
25 changes: 24 additions & 1 deletion third_party/move/move-model/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3311,6 +3311,8 @@
pub used_modules: BTreeSet<ModuleId>,
/// Whether to use `m::T` for representing types, for stable output in docgen
pub use_module_qualification: bool,
/// Var types that are recursive and should appear as `..` in display
pub recursive_vars: Option<BTreeSet<u32>>,
}

impl<'a> TypeDisplayContext<'a> {
Expand All @@ -3324,6 +3326,7 @@
display_type_vars: false,
used_modules: BTreeSet::new(),
use_module_qualification: false,
recursive_vars: None,
}
}

Expand All @@ -3347,6 +3350,7 @@
display_type_vars: false,
used_modules: BTreeSet::new(),
use_module_qualification: false,
recursive_vars: None,

Check warning on line 3353 in third_party/move/move-model/src/ty.rs

View check run for this annotation

Codecov / codecov/patch

third_party/move/move-model/src/ty.rs#L3353

Added line #L3353 was not covered by tests
}
}

Expand All @@ -3356,6 +3360,19 @@
..self
}
}

pub fn map_var_to_self(&self, idx: u32) -> Self {
Self {
recursive_vars: if let Some(existing_set) = &self.recursive_vars {
let mut new_set = existing_set.clone();
new_set.insert(idx);
Some(new_set)

Check warning on line 3369 in third_party/move/move-model/src/ty.rs

View check run for this annotation

Codecov / codecov/patch

third_party/move/move-model/src/ty.rs#L3367-L3369

Added lines #L3367 - L3369 were not covered by tests
} else {
Some(BTreeSet::from([idx]))
},
..self.clone()
}
}
}

/// Helper for type displays.
Expand Down Expand Up @@ -3447,6 +3464,11 @@
}
},
Var(idx) => {
if let Some(recursive_vars) = &self.context.recursive_vars {
if recursive_vars.contains(idx) {
return f.write_str("..");
}

Check warning on line 3470 in third_party/move/move-model/src/ty.rs

View check run for this annotation

Codecov / codecov/patch

third_party/move/move-model/src/ty.rs#L3470

Added line #L3470 was not covered by tests
}
if let Some(ty) = self.context.subs_opt.and_then(|s| s.subs.get(idx)) {
write!(f, "{}", ty.display(self.context))
} else if let Some(ctrs) =
Expand All @@ -3456,9 +3478,10 @@
if ctrs.is_empty() {
f.write_str(&self.type_var_str(*idx))
} else {
let recursive_context = self.context.map_var_to_self(*idx);
let out = ctrs
.iter()
.map(|(_, _, c)| c.display(self.context).to_string())
.map(|(_, _, c)| c.display(&recursive_context).to_string())
.join(" + ");
f.write_str(&out)
}
Expand Down
Loading