Skip to content

Conversation

HenryCROSS
Copy link

@HenryCROSS HenryCROSS commented Jun 9, 2025

Related Issues

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)
    • Originally, the' moon run src/foo' command would compile all packages under the module, even though the package path was specified.
      • That is the reason the defect mentioned in the issue occurred.
    • New changes: The run command will only compile the package indicated by the command.
  • No

Does this PR introduce new dependencies?

  • No
  • Yes (please check binary size changes)

Checklist:

  • Tests added/updated for bug fixes or new features
  • Compatible with Windows/Linux/macOS

Copy link

peter-jerry-ye-code-review bot commented Jun 9, 2025

Potential panic with unwrap() on get_package_by_path

Category
Correctness
Code Snippet
let selected_pkg = module
.get_package_by_path(&PathBuf::from(&package))
.unwrap();
Recommendation
Handle the error case gracefully instead of using unwrap(). Use proper error handling with Context or return early with an error message.
Reasoning
The unwrap() could cause a panic if the package path is invalid. Since this code already has error handling patterns with bail!() and anyhow::Context, it should follow the same pattern for consistency and robustness.

Redundant package lookup - same package retrieved twice

Category
Maintainability
Code Snippet
let selected_pkg = module
.get_package_by_path(&PathBuf::from(&package))
.unwrap();
...
let pkg = module.get_package_by_path_mut(&package).unwrap();
Recommendation
Combine the two package lookups or store the package reference to avoid duplicate work. Consider restructuring to get the mutable reference first and extract the full_name() from it.
Reasoning
The same package is being looked up twice in close proximity, which is inefficient and creates code duplication. This also increases the risk of inconsistent behavior if the module state changes between calls.

Unnecessary PathBuf allocation for package lookup

Category
Performance
Code Snippet
let selected_pkg = module
.get_package_by_path(&PathBuf::from(&package))
.unwrap();
Recommendation
If get_package_by_path accepts &Path or &str, avoid the PathBuf::from() allocation. Use the same parameter type that's used in the subsequent call to get_package_by_path_mut().
Reasoning
Creating a PathBuf allocation just for a lookup when the same variable is used directly in the next call suggests an inconsistency in the API usage that could be optimized.

@HenryCROSS
Copy link
Author

image

Not sure what the reason was when I was trying to sign the form

@HenryCROSS
Copy link
Author

image

Not sure what the reason was when I was trying to sign the form

It is solved.

@HenryCROSS
Copy link
Author

Sorry for the late update, I've been a bit busy recently.

Compared to the code without changes, there are two more errors during the test in my local. I'm not sure what the issue is, and it might take some time to figure out.

test test_cases::test_sub_package ... FAILED
test test_cases::test_native_backend_cc_flags_with_env_override ... FAILED

@Young-Flash
Copy link
Contributor

Sorry for the late update, I've been a bit busy recently.

Compared to the code without changes, there are two more errors during the test in my local. I'm not sure what the issue is, and it might take some time to figure out.

test test_cases::test_sub_package ... FAILED test test_cases::test_native_backend_cc_flags_with_env_override ... FAILED

  • run env UPDATE_EXPECT=1 cargo test and check whether the diff is excepted
  • need a rebase

@HenryCROSS
Copy link
Author

Sorry for the late update, I've been a bit busy recently.
Compared to the code without changes, there are two more errors during the test in my local. I'm not sure what the issue is, and it might take some time to figure out.
test test_cases::test_sub_package ... FAILED test test_cases::test_native_backend_cc_flags_with_env_override ... FAILED

  • run env UPDATE_EXPECT=1 cargo test and check whether the diff is excepted
  • need a rebase

env UPDATE_EXPECT=1 cargo test has a different result, which doesn't have the extra errors.

@HenryCROSS HenryCROSS force-pushed the fix_moon_run branch 2 times, most recently from 7b63291 to 4872036 Compare June 18, 2025 20:32
@Young-Flash
Copy link
Contributor

env UPDATE_EXPECT=1 cargo test will update the test cases, the diff was caused by this PR. so we should check these diff and commit them.

@HenryCROSS
Copy link
Author

env UPDATE_EXPECT=1 cargo test will update the test cases, the diff was caused by this PR. so we should check these diff and commit them.

I ran env UPDATE_EXPECT=1 cargo test for both the main branch and my branch after I updated it.

test result: FAILED. 134 passed; 50 failed; 6 ignored; 0 measured; 0 filtered out;

It seems that all the errors are caused by the main branch rather than the changes introduced in this PR.

@Young-Flash
Copy link
Contributor

need a rebase & use latest toolchain

@HenryCROSS
Copy link
Author

HenryCROSS commented Jun 20, 2025

need a rebase & use latest toolchain

➜  moon git:(fix_moon_run) ✗ rustup check
stable-x86_64-unknown-linux-gnu - Up to date : 1.87.0 (17067e9ac 2025-05-09)
nightly-x86_64-unknown-linux-gnu - Up to date : 1.89.0-nightly (255aa2208 2025-06-19)
➜  moon git:(fix_moon_run) ✗ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/henry/.rustup

installed toolchains
--------------------
stable-x86_64-unknown-linux-gnu (active, default)
nightly-x86_64-unknown-linux-gnu

active toolchain
----------------
name: stable-x86_64-unknown-linux-gnu
active because: it's the default toolchain
installed targets:
  x86_64-unknown-linux-gnu
➜  moon git:(fix_moon_run) ✗ cargo -V   
cargo 1.87.0 (99624be96 2025-05-06)

After I rebased and run env UPDATE_EXPECT=1 cargo test:

failures:
    test_cases::inline_test::test_inline_test_001
    test_cases::inline_test::test_inline_test_002
    test_cases::inline_test::test_inline_test_order
    test_cases::merge_doc_test_and_md_test
    test_cases::moon_check_and_test_single_file
    test_cases::moon_new::test_moon_new_new
    test_cases::moon_test::test_moon_test_hello_exec
    test_cases::moon_test::test_moon_test_hello_exec_fntest
    test_cases::moon_test::test_moon_test_hello_lib
    test_cases::moon_test::test_moon_test_succ
    test_cases::moon_test::test_moon_test_with_local_dep
    test_cases::native_backend_test_filter
    test_cases::targets::test_many_targets
    test_cases::targets::test_many_targets_auto_update_001
    test_cases::targets::test_many_targets_auto_update_002
    test_cases::targets::test_many_targets_auto_update_003
    test_cases::targets::test_many_targets_auto_update_004
    test_cases::test_add_mi_if_self_not_set_in_test_imports
    test_cases::test_blackbox_success
    test_cases::test_expect_test::test_expect_test
    test_cases::test_filter::moon_test_parallelize_should_success
    test_cases::test_filter::moon_test_parallelize_test_filter_should_success
    test_cases::test_filter::test_moon_parallelism
    test_cases::test_filter::test_moon_test_filter_file
    test_cases::test_filter::test_moon_test_filter_index
    test_cases::test_filter::test_moon_test_filter_index_with_auto_update
    test_cases::test_filter::test_moon_test_filter_multi_package
    test_cases::test_filter::test_moon_test_filter_package
    test_cases::test_filter::test_moon_test_filter_package_with_deps
    test_cases::test_filter::test_moon_test_filter_package_with_test_imports
    test_cases::test_in_main_pkg
    test_cases::test_js
    test_cases::test_moon_coverage
    test_cases::test_moon_run_with_cli_args
    test_cases::test_moon_test_patch
    test_cases::test_moon_test_release
    test_cases::test_native_stub_in_pkg_json
    test_cases::test_no_mi_for_test_pkg
    test_cases::test_render_diagnostic_in_patch_file
    test_cases::test_run_md_test
    test_cases::test_snapshot_test
    test_cases::test_snapshot_test_target_js
    test_cases::test_specify_source_dir_001
    test_cases::test_specify_source_dir_002
    test_cases::test_specify_source_dir_with_deps
    test_cases::test_update_expect_failed
    test_cases::test_virtual_pkg
    test_cases::warns::test_alert_list
    test_cases::warns::test_warn_list_dry_run
    test_cases::warns::test_warn_list_real_run

test result: FAILED. 134 passed; 50 failed; 6 ignored; 0 measured; 0 filtered out; finished in 6.95s

$ moon test
Total tests: 1, passed: 1, failed: 0.
[255]
failed: moonc build-package -error-format json ${WORK_DIR}/src/lib/hello_test.mbt ${WORK_DIR}/target/wasm-gc/debug/test/lib/__generated_driver_for_blackbox_test.mbt -o ${WORK_DIR}/target/wasm-gc/debug/test/lib/lib.blackbox_test.core -pkg username/hello/lib_blackbox_test -is-main -std-path $MOON_HOME/lib/core/target/wasm-gc/release/bundle -i ${WORK_DIR}/target/wasm-gc/debug/test/lib/lib.mi:lib -pkg-sources username/hello/lib_blackbox_test:${WORK_DIR}/src/lib -target wasm-gc -g -O0 -source-map -blackbox-test -no-mi -test-mode
Copy link
Contributor

Choose a reason for hiding this comment

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

these diff are not what we expected, have you using latest toolchain ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I left the version in the stream, it is the newest stable toolchain. Also I tried to run it in a clean Ubuntu docker environment and got the same result.

@HenryCROSS
Copy link
Author

It should be fine now. No extra errors are being produced compared to the testing errors from the main branch.

@HenryCROSS HenryCROSS requested a review from Young-Flash July 24, 2025 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants