feat: allow constant interfaces #3718

tserg · 2024-01-03T06:25:43Z

What I did

How I did it

Handle interfaces for check_modifiability and in codegen.

How to verify it

See tests.

Commit message

feat: allow constant interfaces

This PR adds support for interfaces as constants.
- also introduced a mild refactor of `check_modifiability()`

Description for the changelog

Allow interfaces as constants.

Cute Animal Picture

codecov-commenter · 2024-01-03T06:50:34Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (af5c49f) 84.16% compared to head (4924a85) 84.11%.

Files	Patch %	Lines
vyper/semantics/types/base.py	50.00%	1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3718      +/-   ##
==========================================
- Coverage   84.16%   84.11%   -0.06%     
==========================================
  Files          92       92              
  Lines       13133    13141       +8     
  Branches     2926     2927       +1     
==========================================
- Hits        11054    11053       -1     
- Misses       1659     1667       +8     
- Partials      420      421       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

charles-cooper · 2024-01-03T14:14:02Z

tests/functional/codegen/calling_convention/test_default_parameters.py

+FOO: constant(Foo) = Foo(0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF)
+
+@external
+def bar(a: uint256, b: Foo = Foo(0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF)) -> Foo:


it would be good for the tests if bar() and faz() returned something different

do you mean like any constant interface?

yea, like 0xaaaa vs 0xbbbbb

vyper/semantics/analysis/utils.py

charles-cooper

per https://github.com/vyperlang/vyper/pull/3718/files#r1440581290, i think it's going to be cleaner if we can get more annotation into the constant folding pass. i think this can be best accomplished if we interleave constant folding into the dependency resolving loop in the module analyzer. we can also probably clean up the constant folding pass further by implementing it as a separate Visitor implementation. this will separate it from the concerns of VyperNode definitions (which should be more pure and not really "know" how to fold themselves) and also be able to incorporate other information that should belong to later passes (mainly, constants structs and interfaces which only get populated during module analysis).

charles-cooper · 2024-01-03T17:00:57Z

per https://github.com/vyperlang/vyper/pull/3718/files#r1440581290, i think it's going to be cleaner if we can get more annotation into the constant folding pass. i think this can be best accomplished if we interleave constant folding into the dependency resolving loop in the module analyzer. we can also probably clean up the constant folding pass further by implementing it as a separate Visitor implementation. this will separate it from the concerns of VyperNode definitions (which should be more pure and not really "know" how to fold themselves) and also be able to incorporate other information that should belong to later passes (mainly, constants structs and interfaces which only get populated during module analysis).

alright, i performed this refactor in tserg#1. but i think actually a simpler fix to the problem at hand is to just introduce a check_modifiability_for_call() method on structs and interfaces which basically just checks the modifiability of all the arguments to the constructor. this is similar to the technique used for builtins, which is to just check for the "_modifiability" attribute instead of needing an import cycle. (actually for cleanliness, BuiltinFunctionT._modifiability could use the same API, although this might be overkill).

vyper/semantics/types/module.py

+from vyper.semantics.analysis.utils import (
+    check_modifiability,
+    validate_expected_type,
+    validate_unique_method_ids,
+)


vyper/semantics/types/user.py

 from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions
-from vyper.semantics.analysis.utils import validate_expected_type
+from vyper.semantics.analysis.utils import check_modifiability, validate_expected_type


charles-cooper · 2024-01-10T23:24:23Z

can you merge in the latest changes from master? and then i think this should be more or less good to go

…/const_interface

charles-cooper · 2024-01-11T17:47:29Z

vyper/semantics/analysis/utils.py

-        call_type_modifiability = getattr(call_type, "_modifiability", Modifiability.MODIFIABLE)
-        return call_type_modifiability >= modifiability
+        # structs and interfaces
+        if isinstance(call_type, TYPE_T):


it's more complicated but i think this actually should use TYPE_T.check_modifiability_for_call (which can dispatch to something like StructT.ctor_modifiability_for_call). the way you have it written, a struct instance can pass the modifiability check.

…/const_interface

charles-cooper · 2024-01-15T14:19:40Z

vyper/semantics/analysis/utils.py

-        call_type_modifiability = getattr(call_type, "_modifiability", Modifiability.MODIFIABLE)
-        return call_type_modifiability >= modifiability
+        # structs and interfaces
+        if hasattr(call_type, "check_modifiability_for_call"):


as a general comment, i'd like to move these hasattr checks to be more like

if call_type._is_callable: call_type.check_modifiability_for_call(...)

that way we impose more structure on the APIs of callable types. but i think that can be pushed to "future work"

charles-cooper · 2024-01-15T14:21:09Z

vyper/semantics/types/base.py

@@ -334,6 +334,11 @@ def __init__(self, typedef):
    def __repr__(self):
        return f"type({self.typedef})"

+    def check_modifiability_for_call(self, node, modifiability):
+        if hasattr(self.typedef, "_ctor_modifiability_for_call"):


ditto here -- all these hasattr(self.typedef, "_ctor_...") could be more like

if self._is_callable: self.typedef._ctor_modifiability_for_call(...)

charles-cooper · 2024-01-15T14:28:53Z

vyper/semantics/types/module.py

@@ -81,6 +85,9 @@ def _ctor_arg_types(self, node):
    def _ctor_kwarg_types(self, node):
        return {}

+    def _ctor_modifiability_for_call(self, node: vy_ast.Call, modifiability: Modifiability) -> bool:
+        return check_modifiability(node.args[0], modifiability)


this will panic if len(node.args) < 1 (because in some cases, check_modifiability runs before validate_call_args).

charles-cooper · 2024-01-15T14:29:22Z

vyper/semantics/types/user.py

@@ -419,3 +420,6 @@ def _ctor_call_return(self, node: vy_ast.Call) -> "StructT":
            )

        return self
+
+    def _ctor_modifiability_for_call(self, node: vy_ast.Call, modifiability: Modifiability) -> bool:
+        return all(check_modifiability(v, modifiability) for v in node.args[0].values)


same -- will panic if len(node.args) < 1

vyper/semantics/types/function.py

-    validate_expected_type,
-)
+from vyper.semantics.analysis.base import FunctionVisibility, StateMutability, StorageSlot
+from vyper.semantics.analysis.utils import get_exact_type_from_node, validate_expected_type


charles-cooper

looks good. i worked a bit on the panic issue in bf7b346 but i think we can punt on it to a future PR since it's a fairly minor UX thing. (and it's not really a regression since that ordering of check_modifiability before the call to validate_call_args was already latent in the codebase).

tserg added 4 commits January 3, 2024 14:23

handle interface modifiability

3ed97ea

fix codegen

80eacfd

add tests

22c89a9

fix lint

6e1106d

add test

627b4f7

charles-cooper reviewed Jan 3, 2024

View reviewed changes

vyper/semantics/analysis/utils.py Outdated Show resolved Hide resolved

charles-cooper requested changes Jan 3, 2024

View reviewed changes

apply bts suggestion

5910a5d

github-advanced-security bot found potential problems Jan 4, 2024

View reviewed changes

use different addr

116bb7d

tserg added 2 commits January 11, 2024 11:08

Merge branch 'master' of https://github.com/vyperlang/vyper into feat…

8f4b308

…/const_interface

add check_modifiability_for_call to builtins

e7b029b

charles-cooper reviewed Jan 11, 2024

View reviewed changes

tserg added 2 commits January 15, 2024 14:45

apply bts suggestion

0c0f4ae

Merge branch 'master' of https://github.com/vyperlang/vyper into feat…

4924a85

…/const_interface

charles-cooper reviewed Jan 15, 2024

View reviewed changes

github-advanced-security bot found potential problems Jan 15, 2024

View reviewed changes

charles-cooper force-pushed the feat/const_interface branch from bf7b346 to 4924a85 Compare January 15, 2024 14:57

charles-cooper approved these changes Jan 15, 2024

View reviewed changes

charles-cooper changed the title ~~feat: allow interface constants~~ feat: allow constant interfaces Jan 15, 2024

charles-cooper enabled auto-merge (squash) January 15, 2024 15:03

charles-cooper merged commit 88d9c22 into vyperlang:master Jan 15, 2024
164 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow constant interfaces #3718

feat: allow constant interfaces #3718

tserg commented Jan 3, 2024 •

edited by charles-cooper

Loading

codecov-commenter commented Jan 3, 2024 •

edited

Loading

charles-cooper Jan 3, 2024

tserg Jan 4, 2024

charles-cooper Jan 4, 2024

charles-cooper left a comment

charles-cooper commented Jan 3, 2024

charles-cooper commented Jan 10, 2024

charles-cooper Jan 11, 2024

charles-cooper Jan 15, 2024

charles-cooper Jan 15, 2024

charles-cooper Jan 15, 2024

charles-cooper Jan 15, 2024

charles-cooper left a comment

feat: allow constant interfaces #3718

feat: allow constant interfaces #3718

Conversation

tserg commented Jan 3, 2024 • edited by charles-cooper Loading

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

codecov-commenter commented Jan 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-cooper left a comment

Choose a reason for hiding this comment

charles-cooper commented Jan 3, 2024

charles-cooper commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-cooper left a comment

Choose a reason for hiding this comment

tserg commented Jan 3, 2024 •

edited by charles-cooper

Loading

codecov-commenter commented Jan 3, 2024 •

edited

Loading