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

[Bugfix][Compiler-V2] Fix public(package) causing unit test errors #15627

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fEst1ck
Copy link
Contributor

@fEst1ck fEst1ck commented Dec 17, 2024

Description

Fixes #15618.

The transformation of public(packge) to public(friend) currently is only done for modules in the current package, but not the dependencies. This becomes an issue when the generated bytecode of dependencies is used, say in the case unit tests.

We fix this by also doing the transformation to the dependency modules. For dependencies, we cannot perform the transformation properly, as there is no way tell if two non-primary target modules are in the same package (see #13745). So we may friend modules outside the current package. If a dependency contains public(packge) visibility violation, say package A contains calls function in package B that calls a public(package) function in package C, this will not be captured in the unit test as either a compiler error or runtime error. However, each package is compiled with it set as primary target before deployment, so a package with package visibility errors will never hit the chain.

How Has This Been Tested?

All existing test cases. Manually tested the example #15618.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

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

Copy link

trunk-io bot commented Dec 17, 2024

⏱️ 34m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-move-tests 13m 🟩
check-dynamic-deps 7m 🟩🟩🟩
rust-move-tests 5m
rust-cargo-deny 4m 🟩🟩
general-lints 2m 🟩🟩🟩
semgrep/ci 1m 🟩🟩🟩
rust-move-tests 47s
file_change_determinator 38s 🟩🟩🟩
permission-check 13s 🟩🟩🟩
permission-check 9s 🟩🟩🟩

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

Job Duration vs 7d avg Delta
check-dynamic-deps 5m 1m +319%

settingsfeedbackdocs ⋅ learn more about trunk.io

@@ -490,7 +490,7 @@ impl<'env> ModelBuilder<'env> {
let target_modules = self
.env
.get_modules()
.filter(|module_env| module_env.is_primary_target() && !module_env.is_script_module())
.filter(|module_env| module_env.is_primary_target() || module_env.is_target() && !module_env.is_script_module())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we also need include non targets?

@@ -490,7 +490,7 @@ impl<'env> ModelBuilder<'env> {
let target_modules = self
.env
.get_modules()
.filter(|module_env| module_env.is_primary_target() && !module_env.is_script_module())
.filter(|module_env| module_env.is_primary_target() || module_env.is_target() && !module_env.is_script_module())
Copy link

Choose a reason for hiding this comment

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

The operator precedence in this condition may not be doing what's intended. Currently !module_env.is_script_module() only applies to the second part of the OR condition. To ensure script modules are filtered out in both cases, consider restructuring as:

!module_env.is_script_module() && (module_env.is_primary_target() || module_env.is_target())

Spotted by Graphite Reviewer

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Zekun Wang added 2 commits December 17, 2024 17:04
Copy link
Contributor

@vineethk vineethk left a comment

Choose a reason for hiding this comment

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

Have some comments, approving assuming those are addressed.

@@ -3015,12 +3013,12 @@ impl<'env> ModuleEnv<'env> {
}

/// Returns true if functions in the current module can call a public(package) function in the given module.
Copy link
Contributor

Choose a reason for hiding this comment

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

In this PR: https://github.com/aptos-labs/aptos-core/pull/13680/files#diff-d91ccc31f071f5356fd31e9d48dc8372b55377e257f4f2d2a1fdebdafa78a48f, in function_checker.rs, is_target() was changed to is_primary_target().

  1. Among those, for the visibility check cases, can you add a similar TODO (so that once we properly add friend decls for non-primary targets for public(package), we perform visibility checks on dependencies' sources as well, which is the v1 behavior).
  2. For the non-visibility-check cases, can you double check if the change from is_target() to is_primary_target() was valid, and briefly mention why, as a reply here?

!self.is_script_module()
&& !other.is_script_module()
&& other.is_primary_target()
// TODO: fix this when we have a way to check if
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO: fix this when we have a way to check if
// TODO(#13745): fix this when we have a way to check if

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.

[Bug][compiler-v2] public(package) causes unit test errors
2 participants