-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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] Fix parsing of location
in plan_builder of compiler v2
#15312
Conversation
⏱️ 1h 34m total CI duration on this PR
|
location
in plan_builder of compiler v2
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.
Approving assuming comments are addressed.
@@ -505,10 +505,17 @@ fn convert_location(env: &GlobalEnv, attr: Attribute) -> Option<ModuleId> { | |||
match value { | |||
AttributeValue::Name(id, opt_module_name, _sym) => { | |||
let vloc = env.get_node_loc(id); | |||
convert_module_id(env, vloc, opt_module_name) | |||
let module_id_opt = convert_module_id(env, vloc.clone(), opt_module_name); | |||
if !_sym.display(env.symbol_pool()).to_string().is_empty() || module_id_opt.is_none() { |
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.
if !_sym.display(env.symbol_pool()).to_string().is_empty() || module_id_opt.is_none() { | |
if !sym.display(env.symbol_pool()).to_string().is_empty() || module_id_opt.is_none() { |
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.
Also change _sym
-> sym
elsewhere.
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.
Is there a test case corresponding to !_sym.display(env.symbol_pool()).to_string().is_empty()
?
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.
yes, in the test case where we have #[expected_failure(out_of_gas, location=0x1::m::t0)]
and _sym
corresponds to t0
.
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.
And when would it be an empty string?
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.
when it is a module: 0x1::m
. Added a test for this case.
}, | ||
AttributeValue::Value(id, _val) => { | ||
let vloc = env.get_node_loc(id); | ||
let vloc: Loc = env.get_node_loc(id); |
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.
Was this explicit type annotation temporarily added for debugging?
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.
done
b1105c4
to
3bd4f5f
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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 would be nice to better localize the errors, but this is OK.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
This PR closes #14635 by fixing how
location
is parsed in the attribute for move unit tests.How Has This Been Tested?
Existing tests.
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist