aptos-labs / aptos-core

Aptos is a layer 1 blockchain built to support the widespread use of blockchain through better technology and user experience.
https://aptosfoundation.org
Other
6.18k stars 3.66k forks source link

[Bug][compiler-v2] Coverage testing DevEx issues #15117

Open alnoki opened 3 weeks ago

alnoki commented 3 weeks ago

@brmataptos @fEst1ck @georgemitenkov @rahxephon89 @runtian-zhou @vineethk @wrwg

--move-2 flag DevEx

I am trying to run aptos move test --coverage for this commit of an ongoing Move v2 red-black tree implementation, but it is failing with the following trace:

aptos move test --coverage      
INCLUDING DEPENDENCY AptosFramework
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING RedBlackMap
error[E01013]: unsupported language construct
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:6:5
  │
6 │     enum Color has copy, drop {
  │     ^^^^ Move 2 language construct is not enabled: enum types

error[E01013]: unsupported language construct
   ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:60:38
   │
60 │             if (parent_ref_mut.color is Color::Black)
   │                                      ^^ Move 2 language construct is not enabled: `is` expression

error[E01013]: unsupported language construct
   ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:81:70
   │
81 │             if (uncle_index == NIL || (self.nodes[uncle_index].color is Color::Black)) {
   │                                                                      ^^ Move 2 language construct is not enabled: `is` expression

EDIT: Per https://github.com/aptos-labs/aptos-core/issues/15117#issuecomment-2447584475, tests pass for aptos move test --coverage --move-2 --dev, but is there a way to eliminate the need to pass --move-2 all over the place? (now captured at #15124)

Coverage against source is broken

  1. aptos move coverage source --module red_black_map --dev --move-2 is broken (but displaying against bytecode works)
alnoki commented 3 weeks ago

Per https://x.com/wgrieskamp/status/1851647160040948098, I gathered that this requires a --move-2 flag

I just verified that coverage does indeed work with aptos move test --coverage --dev --move-2, so thanks for the quick fix - but is there anyway the CLI could do automatic inference to prevent the requirement of passing --move-2?

Note too that coverage against source is also broken, I've updated the issue description/title accordingly

vineethk commented 3 weeks ago

@alnoki

Could you say in what way is the aptos move coverage source --module .. --move-2 is broken? Did you also generate the coverage information using aptos move test --coverage --move-2?

For some simple examples using enums, using --move-2 for running the tests with coverage, as well as for the source coverage display seems to work.

alnoki commented 3 weeks ago

Could you say in what way is the aptos move coverage source --module .. --move-2 is broken?

@vineethk here's shell output for reproducing

(base) ~ % cd repos/econia-v5/src/move/research/red-black-map/variants/b   
(base) b % aptos move test --coverage --move-2 --dev                         
INCLUDING DEPENDENCY AptosFramework
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING RedBlackMap
warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:4:14
  │
4 │     use std::debug;
  │              ^^^^^ Unused 'use' of alias 'debug'. Consider removing it

Running Move unit tests
[ PASS    ] 0xabc::red_black_map::test_bulk_insertions
[ PASS    ] 0xabc::red_black_map::test_search_insert_cases
Test result: OK. Total tests: 2; passed: 2; failed: 0
warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:4:14
  │
4 │     use std::debug;
  │              ^^^^^ Unused 'use' of alias 'debug'. Consider removing it

warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:5:14
  │
5 │     use std::vector;
  │              ^^^^^^ Unused 'use' of alias 'vector'. Consider removing it

+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 0000000000000000000000000000000000000000000000000000000000000abc::red_black_map
>>> % Module coverage: 96.86
+-------------------------+
| % Move Coverage: 96.86  |
+-------------------------+
Please use `aptos move coverage -h` for more detailed source or bytecode test coverage of this package
{
  "Result": "Success"
}
(base) b % aptos move coverage source --module red_black_map --dev --move-2  
warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:4:14
  │
4 │     use std::debug;
  │              ^^^^^ Unused 'use' of alias 'debug'. Consider removing it

warning: unused alias
  ┌─ /Users/alex/repos/econia-v5/src/move/research/red-black-map/variants/b/sources/red_black_map.move:5:14
  │
5 │     use std::vector;
  │              ^^^^^^ Unused 'use' of alias 'vector'. Consider removing it

thread 'main' panicked at third_party/move/tools/move-coverage/src/source_coverage.rs:333:69:
attempt to subtract with overflow
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
vineethk commented 3 weeks ago

Summary:

Using --move-2 should generally work with aptos move test --coverage and aptos move coverage source --module <module_name> when involving code with Move 2 features.

But there are 2 related issues brought up in the discussions above:

  1. A feature request for making it easier to work with Move 2 programs, without having to explicitly pass --move-2 everywhere. A possibility here is to use Move.toml to specify the use of Move 2, which various Move tools can use directly.
  2. Move coverage tool panics in the scenario given here: https://github.com/aptos-labs/aptos-core/issues/15117#issuecomment-2447687660, which is bug.
alnoki commented 3 weeks ago

@vineethk

  1. A feature request for making it easier to work with Move 2 programs, without having to explicitly pass --move-2 everywhere. A possibility here is to use Move.toml to specify the use of Move 2, which various Move tools can use directly.

Captured at https://github.com/aptos-labs/aptos-core/issues/15124

Move coverage tool panics in the scenario given here: https://github.com/aptos-labs/aptos-core/issues/15117#issuecomment-2447687660, which is bug.

Should this issue suffice for tracking this?

vineethk commented 3 weeks ago

Should this issue suffice for tracking this?

Yes, thank you.