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

fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation #1

Conversation

brmataptos
Copy link
Owner

@brmataptos brmataptos commented Sep 24, 2024

Description

Catch all errors in move-compiler-v2/tests/checking/typing/lambda.move in Compiler V2,
after splitting and gradually commenting out code as lambda[2-5].move,
to avoid shadowing by other errors. Lay groundwork for support of lambdas in various capacities.

Fixes aptos-labs#14633.

  • add checking for function-typed function results in function_checker
  • Change internal_error to error in bytecode_generator and module_generator
    to properly show "error" rather than "bug" if lambdas are mistakenly used as values today.
  • tag some code to show where changes are needed to support lambda: // TODO(LAMBDA)
  • fix unused_params_checker and recursive_struct_checker to tolerate lambda types
  • add experiments to control enabling of various aspects of lambda support:
    • LAMBDA_PARAMS, LAMBDA_RESULTS to allow lambdas as general function params or results
    • LAMBDA_FIELDS - support lambdas in struct fields
    • LAMBDA_VALUES - support lambdas in general-purpose values
    • may add LAMBDA_TYPE_PARAMS later
  • update lambda-lifting test config to use those flags rather than disabling checks
  • add a result_type_loc field to move-model's FunctionData (and model-builder's FunEntry)
    to more precisely locate errors in function result types.
  • Fix GlobalEnv::internal_dump_env to use function- and struct-specific type contexts
    so that types are shown with correct type parameter names.
  • Check that struct fields are not functions.

How Has This Been Tested?

Running the usual tests.

Key Areas to Review

Note that a few errors are caught quite late, in bytecode-generator,
and the order in which we catch errors may be a bit surprising to
users (errors in exp_builder come first, then errors caught by
function_checker, then (much later) bytecode_generator.).
Actually supporting particular subsets of the "experiments" mentioned
above would be difficult, but it seems useful to have the labels to
help understand just what features are supported by the implementation
as we go.

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

larry-aptos and others added 20 commits September 19, 2024 07:11
…aptos-labs#14578)

* add options and plumbing for compile_verify_code, warn_deprecated, etc., and add final set of V1 tests to V2, except flavor tests (evm/async)
* move around and edit/split tests to test same functionality in V2 as V1
Patches the indexer processors for 4.2.0
…mbda parameters (aptos-labs#14254)

Allowing declared type annotations for lambda parameters.

Fixes aptos-labs#6922.
The old version relies on a deprecated version of Node.js
V3 runs on a deprecated version of Node.js (16) and Github has now
started forcing Node.js 20.
… points for more complete lambda implementation
Copy link
Owner Author

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

Join @brmataptos and the rest of your teammates on Graphite Graphite

@brmataptos brmataptos marked this pull request as ready for review September 24, 2024 20:53
@brmataptos brmataptos merged commit f0acc32 into main Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants