fix[lang]: transitive exports #3888
Conversation
put exports into the module type namespace
charles-cooper
requested review from
fubuloubu,
tserg,
cyberthirst and
pcaversaccio
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3888 +/- ##
==========================================
- Coverage 86.38% 86.32% -0.07%
==========================================
Files 92 92
Lines 14022 14025 +3
Branches 3080 3082 +2
==========================================
- Hits 12113 12107 -6
- Misses 1482 1490 +8
- Partials 427 428 +1 ☔ View full report in Codecov by Sentry. |
| @@ -131,9 +128,7 @@ def foo() -> uint256: | |||
| assert c.foo() == 5 | |||
|
|
|||
|
|
|||
| # not sure if this one should work | |||
There was a problem hiding this comment.
For the the future we should track these cases (i.e. ambiguous spec) somewhere properly, since we obviously forgot about it and thanks to feedback rediscovered it.
Nit: if we use xfail, we should also add the reason attribute instead of a comment to be consistent:
There was a problem hiding this comment.
i don't really get this -- it's easy to grep the codebase or to run with pytest --runxfail. i guess you mean we should be doing this on a regular basis?
There was a problem hiding this comment.
it's easy to grep the codebase or to run with
pytest --runxfail. i guess you mean we should be doing this on a regular basis?
My point is not that it's easy, but that we forget about certain xfail tests that are part of a bigger features (like in this case) and we forget to address it before the release.
| # not sure if this one should work | ||
| @pytest.mark.xfail(reason="ambiguous spec") | ||
| def test_recursive_export(make_input_bundle, get_contract): | ||
| def test_transitive_export(make_input_bundle, get_contract): | ||
| lib1 = """ | ||
| @external |
There was a problem hiding this comment.
Nit: this should be @pure
There was a problem hiding this comment.
the current test for transitive exports is pretty limited. Is there a smart way, where we can fuzz different nestedness & visibility decorators?
There was a problem hiding this comment.
hmm i don't think we need to "fuzz" so to speak, and the external/internal distinction should be tested in other tests already. as for nesting, we could maybe set up a chaining test for like 1-5 levels of nesting.
There was a problem hiding this comment.
yeah, that sounds reasonable to me.
this commit fixes transitive exports, i.e. where `module1` exports `module2.foo`, but it was previously not exportable from `module1`. this is because previously exported functions were not added to the `ModuleT`'s `exposed_functions` list. a test had previously been written for this case, but it had been marked xfail since the desired behavior was not clear until user feedback was received. this commit removes the `xfail` marker from that test case.
What I did
fix #3881
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture