[Compiler-v2] Check recursive definition for constants #14741

rahxephon89 · 2024-09-24T20:43:10Z

Description

This PR

adds checks to only allow referring other constants in constant definition when the language version is at least 2.0.
rejects recursive definition of constants.

close #14648

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Update existing test cases;
Add a new test to checking-lang-v1

Key Areas to Review

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-09-24T20:43:14Z

⏱️ 6h 32m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	45m	🟥 🟩
forge-compat-test / forge	28m	⬜ 🟩
forge-framework-upgrade-test / forge	23m	⬜ 🟩
forge-e2e-test / forge	21m	⬜ 🟩
rust-cargo-deny	18m	🟩 ⬜ 🟩 ⬜ 🟩 (+7 more)
general-lints	16m	🟩 ⬜ 🟩 🟩 🟩 (+7 more)
rust-move-unit-coverage	16m	🟩
rust-move-unit-coverage	16m	🟩
check-dynamic-deps	12m	🟩 🟩 🟩 🟩 🟩 (+7 more)
rust-move-unit-coverage	12m	🟩
rust-move-unit-coverage	12m	🟩
rust-move-unit-coverage	12m	🟩
rust-move-unit-coverage	11m	🟩
rust-move-unit-coverage	10m	🟩
rust-move-tests	10m	🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
general-lints	25s	2m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

rahxephon89 · 2024-09-24T20:43:34Z

[Compiler-v2] Check recursive definition for constants #14741 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rahxephon89 and the rest of your teammates on Graphite

codecov · 2024-09-24T21:08:18Z

Codecov Report

Attention: Patch coverage is 98.55072% with 2 lines in your changes missing coverage. Please review.

Project coverage is 59.9%. Comparing base (8689b9a) to head (a4654c6).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...arty/move/move-model/src/builder/module_builder.rs	99.2%	1 Missing ⚠️
third_party/move/move-model/src/constant_folder.rs	85.7%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #14741       +/-   ##
===========================================
- Coverage    72.7%    59.9%    -12.9%     
===========================================
  Files        2397      852     -1545     
  Lines      482334   207601   -274733     
===========================================
- Hits       350819   124363   -226456     
+ Misses     131515    83238    -48277

Flag	Coverage Δ
	`59.9% <98.5%> (-12.9%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vineethk · 2024-09-24T23:09:29Z

third_party/move/move-compiler/src/expansion/ast.rs

+impl Exp_ {
+    /// Get all names from an expression
+    /// only recursively check on expression types supported in constant definition.
+    pub fn get_names_for_const_exp(&self) -> HashSet<Name> {


Could we please use a BTreeSet instead? Looks like we iterate through this hashset later, and such iteration order is not deterministic.

Suggested change

pub fn get_names_for_const_exp(&self) -> HashSet<Name> {

pub fn get_names_from_const_exp(&self) -> HashSet<Name> {

vineethk · 2024-09-24T23:13:42Z

third_party/move/move-compiler/src/expansion/ast.rs

+                    }
+                }
+            },
+            _ => {},


Assuming this is only called in the context of const expressions, would it be better to have an internal error/ICE here?

If you decide to go the route of adding an error, then the Block case has some silently skipped conditions too.

I moved the function internally into analyze_constant to make sure it will not be called by other functions. Then, all other cases will be handled by is_valid_for_constant.

vineethk · 2024-09-24T23:14:27Z

third_party/move/move-compiler/src/expansion/ast.rs

+impl Exp_ {
+    /// Get all names from an expression
+    /// only recursively check on expression types supported in constant definition.
+    pub fn get_names_for_const_exp(&self) -> HashSet<Name> {


Suggested change

pub fn get_names_for_const_exp(&self) -> HashSet<Name> {

pub fn get_names_from_const_exp(&self) -> HashSet<Name> {

vineethk · 2024-09-24T23:16:35Z

third_party/move/move-compiler/src/expansion/ast.rs

+                names.insert(*access.value.get_name());
+            },
+            Self::Call(_, _, _, exp_vec) | Self::Vector(_, _, exp_vec) => {
+                let _ = exp_vec.value.iter().map(&mut add_names);


Is for_each() perhaps better suited here?

vineethk · 2024-09-24T23:27:28Z

third_party/move/move-model/src/builder/module_builder.rs

+        key: &PA::ConstantName,
+        constant_map: &UniqueMap<PA::ConstantName, EA::Constant>,
+        visiting: &mut HashSet<PA::ConstantName>,
+        visited: &mut HashSet<PA::ConstantName>,


Might be good to add a brief note about params like visiting, visited.

vineethk · 2024-09-24T23:31:32Z

third_party/move/move-model/src/builder/module_builder.rs

+                if !self.parent.const_table.contains_key(&qsym) {
+                    continue;
+                }
+                self.check_language_version(


Do we have this feature mentioned in the Move book (i.e., since 2.0)?

no, in the move book we are still saying it is not supported.

third_party/move/move-model/src/builder/module_builder.rs

vineethk · 2024-09-24T23:45:10Z

third_party/move/move-model/src/builder/module_builder.rs

+                    LanguageVersion::V2_0,
+                );
+                if visited.contains(&const_name) {
+                    return;


Why would do this early return? Should we not continue instead, and see if the other names lead to errors? For example:

const X: u64 = 1; const Y: u64 = X + Y;

vineethk · 2024-09-24T23:46:46Z

third_party/move/move-model/src/builder/module_builder.rs

+                    self.parent
+                        .env
+                        .error(&loc, "Found recursive definition of a constant");
+                    return;


Perhaps add a comment that we return early here before exploring other names to limit such errors to one per const definition?

vineethk · 2024-09-24T23:49:38Z

third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.exp

+error: Found recursive definition of a constant
+  ┌─ tests/checking/typing/recursive_constant.move:4:5
+  │
+4 │     const Y: u64 = X;


Could we add some more tests for the recursive check?

self recursion

recursion with a more complicated cycle involving expressions

vineethk · 2024-09-24T23:55:35Z

third_party/move/move-compiler/src/expansion/ast.rs

+            },
+            Self::Block(seq) => {
+                for s in seq.iter() {
+                    if let SequenceItem_::Seq(exp) = &s.value {


Interesting that we allow sequences in const defintions. Is there any use case for this? Should it be disallowed?

Can this sequence contain local bindings that could clash with the const names?

Sequence such a { 3 + 8; Z } is allowed but the one with local bindings will be translated into a Block expression, which is not allowed according to here.

rahxephon89 · 2024-09-25T06:36:40Z

third_party/move/move-model/src/constant_folder.rs

@@ -328,9 +328,17 @@ impl<'env> ConstantFolder<'env> {
                    O::Neq => val0
                        .equivalent(val1)
                        .map(|equivalence| V(id, Bool(!equivalence)).into_exp()),
-                    _ => self.constant_folding_error(id, |_| {


The error can be raised when a constant is not evaluated. Thus modify the error msg here.

vineethk

LGTM, left minor comments.

vineethk · 2024-09-25T15:38:27Z

third_party/move/move-model/src/builder/module_builder.rs

+            self.parent.env.diag_with_labels(
+                Severity::Error,
+                &loc,
+                &format!("Found recursive definition of a constant {}", key),


Suggested change

&format!("Found recursive definition of a constant {}", key),

&format!("Found recursive definition of a constant `{}`", key),

nit: Also may be add: "; cycle formed by definitions below" or similar.

vineethk · 2024-09-25T15:38:50Z

third_party/move/move-model/src/builder/module_builder.rs

+                visiting[index..]
+                    .to_vec()
+                    .iter()
+                    .map(|(name, loc)| (loc.clone(), format!("{} is defined", name)))


Suggested change

.map(|(name, loc)| (loc.clone(), format!("{} is defined", name)))

.map(|(name, loc)| (loc.clone(), format!("`{}` is defined here", name)))

third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/refer_other_constants.exp

third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.move

third_party/move/move-model/src/builder/module_builder.rs

brmataptos

LGTM

github-actions · 2024-09-26T22:15:37Z

✅ Forge suite `realistic_env_max_load` success on `a4654c6863ed7f332a367c2c56d19399b729fa4c`

two traffics test: inner traffic : committed: 14063.07 txn/s, latency: 2826.37 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5347120
two traffics test : committed: 100.00 txn/s, latency: 1644.79 ms, (p50: 1500 ms, p70: 1600, p90: 1700 ms, p99: 7600 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.284, avg: 0.238", "QsPosToProposal: max: 1.143, avg: 1.115", "ConsensusProposalToOrdered: max: 0.335, avg: 0.300", "ConsensusOrderedToCommit: max: 0.426, avg: 0.406", "ConsensusProposalToCommit: max: 0.726, avg: 0.705"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.08s no progress at version 2386639 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.37s no progress at version 2386637 (avg 8.37s) [limit 15].
Test Ok

github-actions · 2024-09-26T22:17:17Z

✅ Forge suite `framework_upgrade` success on `b6187e1794550f4a6b25cab0e5d40188574241da` ==> `a4654c6863ed7f332a367c2c56d19399b729fa4c`

Compatibility test results for b6187e1794550f4a6b25cab0e5d40188574241da ==> a4654c6863ed7f332a367c2c56d19399b729fa4c (PR)
Upgrade the nodes to version: a4654c6863ed7f332a367c2c56d19399b729fa4c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1203.52 txn/s, submitted: 1206.37 txn/s, failed submission: 2.84 txn/s, expired: 2.84 txn/s, latency: 2852.53 ms, (p50: 2400 ms, p70: 2700, p90: 5100 ms, p99: 6400 ms), latency samples: 101540
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1116.99 txn/s, submitted: 1118.78 txn/s, failed submission: 1.78 txn/s, expired: 1.78 txn/s, latency: 2696.77 ms, (p50: 2400 ms, p70: 2900, p90: 4600 ms, p99: 6200 ms), latency samples: 100320
5. check swarm health
Compatibility test for b6187e1794550f4a6b25cab0e5d40188574241da ==> a4654c6863ed7f332a367c2c56d19399b729fa4c passed
Upgrade the remaining nodes to version: a4654c6863ed7f332a367c2c56d19399b729fa4c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1120.00 txn/s, submitted: 1122.94 txn/s, failed submission: 2.94 txn/s, expired: 2.94 txn/s, latency: 2759.60 ms, (p50: 2400 ms, p70: 3000, p90: 5400 ms, p99: 6900 ms), latency samples: 99080
Test Ok

github-actions · 2024-09-26T22:22:05Z

✅ Forge suite `compat` success on `b6187e1794550f4a6b25cab0e5d40188574241da` ==> `a4654c6863ed7f332a367c2c56d19399b729fa4c`

Compatibility test results for b6187e1794550f4a6b25cab0e5d40188574241da ==> a4654c6863ed7f332a367c2c56d19399b729fa4c (PR)
1. Check liveness of validators at old version: b6187e1794550f4a6b25cab0e5d40188574241da
compatibility::simple-validator-upgrade::liveness-check : committed: 13126.13 txn/s, latency: 2581.10 ms, (p50: 2100 ms, p70: 2200, p90: 4000 ms, p99: 8700 ms), latency samples: 431060
2. Upgrading first Validator to new version: a4654c6863ed7f332a367c2c56d19399b729fa4c
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 1583.87 txn/s, latency: 17323.17 ms, (p50: 19500 ms, p70: 23800, p90: 27500 ms, p99: 28600 ms), latency samples: 55720
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6289.20 txn/s, latency: 4774.57 ms, (p50: 5000 ms, p70: 5100, p90: 5200 ms, p99: 5400 ms), latency samples: 233080
3. Upgrading rest of first batch to new version: a4654c6863ed7f332a367c2c56d19399b729fa4c
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6024.31 txn/s, latency: 4691.87 ms, (p50: 5300 ms, p70: 5500, p90: 5900 ms, p99: 6100 ms), latency samples: 112560
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6157.14 txn/s, latency: 5220.45 ms, (p50: 5600 ms, p70: 5700, p90: 7100 ms, p99: 7300 ms), latency samples: 206820
4. upgrading second batch to new version: a4654c6863ed7f332a367c2c56d19399b729fa4c
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11298.49 txn/s, latency: 2389.65 ms, (p50: 2400 ms, p70: 2800, p90: 3200 ms, p99: 3400 ms), latency samples: 198880
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10216.12 txn/s, latency: 3076.57 ms, (p50: 2600 ms, p70: 3100, p90: 5700 ms, p99: 8300 ms), latency samples: 376400
5. check swarm health
Compatibility test for b6187e1794550f4a6b25cab0e5d40188574241da ==> a4654c6863ed7f332a367c2c56d19399b729fa4c passed
Test Ok

rahxephon89 changed the title ~~check recursive in constant definition~~ [Compiler-v2] Check recursive definition for constants Sep 24, 2024

rahxephon89 force-pushed the teng/fix-14648 branch from 4da136c to 63626eb Compare September 24, 2024 20:56

rahxephon89 marked this pull request as ready for review September 24, 2024 22:09

rahxephon89 requested review from brmataptos, vineethk and wrwg September 24, 2024 22:09

vineethk reviewed Sep 24, 2024

View reviewed changes

rahxephon89 force-pushed the teng/fix-14648 branch from 63626eb to 7b47bab Compare September 24, 2024 23:31

rahxephon89 requested a review from vineethk September 24, 2024 23:33

vineethk reviewed Sep 24, 2024

View reviewed changes

rahxephon89 marked this pull request as draft September 25, 2024 00:40

rahxephon89 force-pushed the teng/fix-14648 branch 3 times, most recently from 79a5bcf to 794ea3d Compare September 25, 2024 06:27

rahxephon89 marked this pull request as ready for review September 25, 2024 06:35

rahxephon89 requested a review from vineethk September 25, 2024 06:35

rahxephon89 commented Sep 25, 2024

View reviewed changes

vineethk approved these changes Sep 25, 2024

View reviewed changes

rahxephon89 force-pushed the teng/fix-14648 branch from 794ea3d to 40be542 Compare September 25, 2024 19:48

brmataptos reviewed Sep 26, 2024

View reviewed changes

rahxephon89 force-pushed the teng/fix-14648 branch from 40be542 to a9e5f45 Compare September 26, 2024 03:50

rahxephon89 requested a review from brmataptos September 26, 2024 03:52

brmataptos approved these changes Sep 26, 2024

View reviewed changes

rahxephon89 added 3 commits September 26, 2024 14:24

check recursive in constant definition

555edf2

add tests

2c6fa04

add tests for duplicated constants

f5f1d26

rahxephon89 force-pushed the teng/fix-14648 branch from a9e5f45 to f5f1d26 Compare September 26, 2024 21:25

rahxephon89 enabled auto-merge (squash) September 26, 2024 21:25

rahxephon89 disabled auto-merge September 26, 2024 21:26

This comment has been minimized.

Sign in to view

fix

a4654c6

rahxephon89 enabled auto-merge (squash) September 26, 2024 21:48

This comment has been minimized.

Sign in to view

rahxephon89 merged commit 63de0c5 into main Sep 26, 2024
92 of 97 checks passed

rahxephon89 deleted the teng/fix-14648 branch September 26, 2024 22:22

	pub fn get_names_for_const_exp(&self) -> HashSet<Name> {
	pub fn get_names_from_const_exp(&self) -> HashSet<Name> {

	&format!("Found recursive definition of a constant {}", key),
	&format!("Found recursive definition of a constant `{}`", key),

	.map(\|(name, loc)\| (loc.clone(), format!("{} is defined", name)))
	.map(\|(name, loc)\| (loc.clone(), format!("`{}` is defined here", name)))

[Compiler-v2] Check recursive definition for constants #14741

[Compiler-v2] Check recursive definition for constants #14741

Conversation

rahxephon89 commented Sep 24, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Sep 24, 2024 • edited Loading

rahxephon89 commented Sep 24, 2024

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahxephon89 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

vineethk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brmataptos left a comment

Choose a reason for hiding this comment

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.

github-actions bot commented Sep 26, 2024

✅ Forge suite realistic_env_max_load success on a4654c6863ed7f332a367c2c56d19399b729fa4c

github-actions bot commented Sep 26, 2024

✅ Forge suite framework_upgrade success on b6187e1794550f4a6b25cab0e5d40188574241da ==> a4654c6863ed7f332a367c2c56d19399b729fa4c

github-actions bot commented Sep 26, 2024

✅ Forge suite compat success on b6187e1794550f4a6b25cab0e5d40188574241da ==> a4654c6863ed7f332a367c2c56d19399b729fa4c

rahxephon89 commented Sep 24, 2024 •

edited

Loading

trunk-io bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

rahxephon89 Sep 25, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `a4654c6863ed7f332a367c2c56d19399b729fa4c`

✅ Forge suite `framework_upgrade` success on `b6187e1794550f4a6b25cab0e5d40188574241da` ==> `a4654c6863ed7f332a367c2c56d19399b729fa4c`

✅ Forge suite `compat` success on `b6187e1794550f4a6b25cab0e5d40188574241da` ==> `a4654c6863ed7f332a367c2c56d19399b729fa4c`