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.

Sign up for GitHub

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

Jump to bottom

[compiler][trivial] fix type instantiation for receiver style syntax #16095

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

[compiler][trivial] fix type instantiation for receiver style syntax #16095

wants to merge 2 commits into from

Conversation

rahxephon89
Copy link
Contributor

@rahxephon89 rahxephon89 commented Mar 10, 2025 •
edited
Loading

Description

This PR adds the logic to run finalize_types before and after post_process_body and only generates error in the second time so that type can be instantiated in receiver style syntax before finalizing types.

Close #16091
Close #16057

How Has This Been Tested?

  1. existing tests;
  2. added a new test.

Key Areas to Review

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

@trunk-io Trunk.io
Copy link

trunk-io bot commented Mar 10, 2025 •
edited
Loading

⏱️ 21m total CI duration on this PR
Job Cumulative Duration Recent Runs
check-dynamic-deps 10m 🟩 🟩 🟩 🟩 
rust-cargo-deny 7m 🟩 🟩 🟩 🟩 
general-lints 2m 🟩 🟩 🟩 🟩 
semgrep/ci 1m 🟩 🟩 🟩 🟩 
file_change_determinator 42s 🟩 🟩 🟩 🟩 
permission-check 10s 🟩 🟩 🟩 🟩 
permission-check 10s 🟩 🟩 🟩 🟩 

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@rahxephon89 rahxephon89 changed the title [WIP][compiler] fix type instantiation bugs [WIP][compiler] fix type instantiation bugs in receiver style syntax Mar 10, 2025
@rahxephon89 rahxephon89 changed the title [WIP][compiler] fix type instantiation bugs in receiver style syntax [WIP][compiler][trivial] fix type instantiation bugs in receiver style syntax Mar 10, 2025
@rahxephon89 rahxephon89 changed the title [WIP][compiler][trivial] fix type instantiation bugs in receiver style syntax [compiler][trivial] fix type instantiation bugs in receiver style syntax Mar 10, 2025
@rahxephon89 rahxephon89 changed the title [compiler][trivial] fix type instantiation bugs in receiver style syntax [compiler][trivial] fix type instantiation in receiver style syntax Mar 10, 2025
@rahxephon89 rahxephon89 changed the title [compiler][trivial] fix type instantiation in receiver style syntax [compiler][trivial] fix type instantiation for receiver style syntax Mar 10, 2025
@rahxephon89 rahxephon89 marked this pull request as ready for review March 10, 2025 21:47
@rahxephon89 rahxephon89 requested review from vineethk and wrwg March 10, 2025 21:47
wrwg
wrwg reviewed Mar 11, 2025
View reviewed changes
third_party/move/move-model/src/builder/module_builder.rs Outdated
let translated = et.post_process_body(result.into_exp());
et.finalize_types();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not looking right. Finalization has to happen before post processing since this needs to have the ground types, it is not place by accident in this order. Its also happening inside of post process for some special cases like here. Please look closer what is wrong here to find the right fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that finalize_types generated the error before running post_process_body. I updated the code so that when finalize_types is run before post_process_body, error will not be generated.

@rahxephon89 rahxephon89 force-pushed the teng/fix-16091 branch 2 times, most recently from a4aeca4 to 2331121 Compare March 11, 2025 04:04
@rahxephon89 rahxephon89 requested a review from wrwg March 11, 2025 04:04
wrwg
wrwg reviewed Mar 12, 2025
View reviewed changes
third_party/move/move-model/src/builder/module_builder.rs
@@ -1573,8 +1573,9 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> {
}
let access_specifiers = et.translate_access_specifiers(&def.access_specifiers);
let result = et.translate_seq(&loc, seq, &result_type, &ErrorMessageContext::Return);
et.finalize_types();
et.finalize_types(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment before this call, like "Run type inference finalization so post processing has all available type information, but do not report errors yet because receiver functions can add more type bindings."

Then add to the 2nd call "Run finalization again, this time with reporting errors."

Also look out in post-process and remove code, as we do not need to do the ad-hoc type specialization there any longer which I added to the receiver processing, since we run now finalization a 2nd time.

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

rahxephon89
rahxephon89 commented Mar 12, 2025
View reviewed changes
third_party/move/move-compiler-v2/tests/checking/receiver/call_errors.move
@@ -24,6 +24,5 @@ module 0x42::m {
s.receiver_ref_mut(1u8);
s.receiver_ref_mut::<u8>(1);
s.receiver_ref_mut::<u8, u8>(1);
s.receiver_needs_type_args(x)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add this to a separate test case because this error will be hidden by previous ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is caused by removing adhoc finalize_type in post_process_receiver_call.

rahxephon89
rahxephon89 commented Mar 12, 2025
View reviewed changes
third_party/move/move-model/src/builder/exp_builder.rs
@@ -2139,12 +2141,8 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
if ok {
receiver_param_type = subs.specialize(&receiver_param_type);
self.subs = subs;
let inst = inst
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove instantiation here because finalize_types will be called later.

@rahxephon89 rahxephon89 requested a review from wrwg March 12, 2025 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vineethk vineethk vineethk approved these changes

@wrwg wrwg Awaiting requested review from wrwg

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@rahxephon89 @vineethk @wrwg