Mapping arguments and returns #4798
Conversation
libsolidity/analysis/TypeChecker.cpp
Outdated
| !type(*var)->canLiveOutsideStorage() && | ||
| !( | ||
| (_function.visibility() <= FunctionDefinition::Visibility::Internal) && | ||
| type(*var)->category() == Type::Category::Mapping |
There was a problem hiding this comment.
We might want to discuss, whether not living outside storage is a problem for any type in internal functions.
There was a problem hiding this comment.
Will create a new issue for removing this check and thinking about canLiveOutsideStorage in general.
| function f() internal pure returns (mapping(uint=>uint) storage r) { } | ||
| } | ||
| // ---- | ||
| // TypeError: (53-82): This variable is of mapping type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error. |
There was a problem hiding this comment.
Since the actual control flow analysis is extensively tested for storage returns, I would say that it's sufficient to only check that mappings are considered at all, instead of duplicating all tests in syntaxTests/controlFlow/storageReturn.
Codecov Report
@@ Coverage Diff @@
## develop #4798 +/- ##
===========================================
+ Coverage 28.35% 87.7% +59.34%
===========================================
Files 314 314
Lines 30781 31032 +251
Branches 3676 3675 -1
===========================================
+ Hits 8728 27216 +18488
+ Misses 21380 2566 -18814
- Partials 673 1250 +577
|
ekpyron
force-pushed
the
mappingArgumentsAndReturns
branch
from
9f5b82a
to
df9c2eb
Compare
Changelog.md
Outdated
| @@ -71,6 +72,7 @@ Language Features: | |||
| * General: Support ``pop()`` for storage arrays. | |||
| * General: Scoping rules now follow the C99-style. | |||
| * General: Allow ``enum``s in interfaces. | |||
| * General: Allow ``mapping`` arguments and return values in all internal functions. | |||
There was a problem hiding this comment.
Perhaps we should clarify that this is only about storage pointers.
| @@ -147,7 +150,9 @@ void ControlFlowAnalyzer::checkUnassignedStorageReturnValues( | |||
| m_errorReporter.typeError( | |||
| returnVal->location(), | |||
| ssl, | |||
| "This variable is of storage pointer type and might be returned without assignment and " | |||
| string("This variable is of ") + | |||
| (returnVal->type()->category() == Type::Category::Mapping ? "mapping" : "storage pointer") + | |||
There was a problem hiding this comment.
I think we can both call them storage pointers.
| @@ -0,0 +1,6 @@ | |||
| library L { | |||
| function f(mapping(uint => uint) storage) public pure { | |||
There was a problem hiding this comment.
Syntax highlighting is off here :)
ekpyron
force-pushed
the
mappingArgumentsAndReturns
branch
from
df9c2eb
to
440dbaf
Compare
ekpyron
force-pushed
the
mappingArgumentsAndReturns
branch
from
fc3c63b
to
dfcfc4c
Compare
Fixes #4670. Fixes #4641.
Related: #4635.
This is a compound PR now, but I don't think it makes too much sense to split it up, since the changes are highly related (I will split, if anyone disagrees, though).
The code changes are minimal, but we have to be sure that the tests (in particular the semantics tests) are sufficient, so let me know if you have test cases you think I should add.