Allow named parameters in mapping types #13384

zemse · 2022-08-11T11:56:12Z

Allows providing an optional name/identifier just after the key type and the value type.
Identifier defaults to empty string "".
The optional identifier if provided is set to the "name" field in JSON ABI.
Test cases are added for parser and ABI generator

Please let me know of any changes that are needed.

zemse · 2022-08-13T11:49:54Z

test/libsolidity/errorRecoveryTests/do_not_delete_at_error.sol

@@ -10,4 +10,6 @@ contract Error2 {
 	mapping (address => uint balances; // missing ) before "balances"
 }
 // ----
-// ParserError 6635: (417-425): Expected ')' but got identifier
+// ParserError 6635: (425-426): Expected ')' but got ';'


Some existing error recovery tests change their behavior because a syntax that was illegal before (i.e. ability to place an identifier next to type in mapping) is legal now, hence the parser passes further and generates a different error.

hrkrshnn

Looks quite good! Some small comments.

We were wondering with the annotations, if it makes sense to have the function call options syntax for mappings. For example, for mapping (address user => uint balance) balanceOf, would the syntax balanceOf[{user: 0xaddress}] be allowed? It's not needed to implement it in this PR, but worth thinking about.

Also, it's missing a changelog entry.

libsolidity/ast/AST.h

libsolidity/parsing/Parser.cpp

test/libsolidity/ABIJson/mapping.sol

test/libsolidity/syntaxTests/parsing/mapping_with_names_5.sol

hrkrshnn · 2022-08-17T11:02:48Z

Can you add a semantic test as well? https://github.com/ethereum/solidity/blob/develop/test/libsolidity/semanticTests/getters/mapping.sol here is an example

hrkrshnn · 2022-08-17T11:49:38Z

Could you also add an example in the documentation?

zemse · 2022-08-18T05:32:53Z

Thanks for the review! I've made the changes.

function call options syntax for mappings

This might be tricky for nested mappings: allowances[{owner: 0xAddr}][{spender: 0xAddr}]. Somehow having it together could be good allowances[{owner: 0xAddr, spender: 0xAddr}], however the names are optional hence this together syntax might not work. But yeah having it makes sense for improving code readability, does not affect any ABI or bytecode. I'd prefer adding it in a separate PR unless it is needed in this PR.

Marenz

already looking very good.
Additionally to the comments, I am wondering if it makes sense to make it an optional<ASTPointer<ASTString>>.
Would love to hear @cameel and @christianparpart on that topic

docs/types/mapping-types.rst

libsolidity/parsing/Parser.cpp

cameel · 2022-08-24T15:23:35Z

Additionally to the comments, I am wondering if it makes sense to make it an optional<ASTPointer<ASTString>>.

Depends if we prefer to treat it as a missing name or an empty name. That will show up in the AST.

But overall optional would make it clearer that this is even possible. And since names need to be unique, having to treat "" as an exception is not that great.

Also, I can't see any place in the AST where we'd have a single optional name but in a few cases we do have vectors of names and that's closer to optional than to having an empty string when the name is missing.

zemse · 2022-09-12T18:21:27Z

I have rebased over the latest develop branch to fix the conflicts.

Edit: I'm not able to request reviews from multiple people. cc: @hrkrshnn, @Marenz, @cameel, @nishant-sachdeva. (Sorry for tagging).

hrkrshnn · 2022-09-25T20:18:27Z

Will rebase now

Changelog.md

docs/types/mapping-types.rst

hrkrshnn · 2022-09-25T20:25:40Z

libsolidity/analysis/DeclarationTypeChecker.cpp

 	// Convert key type to memory.
 	keyType = TypeProvider::withLocationIfReference(DataLocation::Memory, keyType);

 	// Convert value type to storage reference.
 	valueType = TypeProvider::withLocationIfReference(DataLocation::Storage, valueType);
-	_mapping.annotation().type = TypeProvider::mapping(keyType, valueType);
+	_mapping.annotation().type = TypeProvider::mapping(keyType, valueType, keyName, valueName);


Nit: I would have preferred the order to be keyType, KeyName, valueType, valueName. But no strong argument on changing this.

libsolidity/ast/AST.h

libsolidity/ast/ASTJsonImporter.cpp

docs/types/mapping-types.rst

- Add changelog

zemse · 2022-11-20T19:32:34Z

Summary of the changes done:

Removed the commit ee0fee7 that disallowed param for mapping value types.
Added more tests in 4aedd77.
Rebased over upstream/develop branch. (For comparison of changes before & after the force push, I've pushed backup branch)

nikola-matic · 2022-11-23T14:26:54Z

test/libsolidity/syntaxTests/parsing/mapping_with_names_conflict_3.sol

+}
+// ----
+// DeclarationError 5609: (45-84): Conflicting parameter name "owner" in mapping.
+// DeclarationError 1809: (20-85): Conflicting parameter name "owner" in recursive mapping.


~~Why does this one have a different error code, i.e. 1809 vs 5609?~~ Ah OK, it's a different error in case the nested type is a mapping, so you're differentiating between conflicting key -> key and key -> value names. The error message is the same though, which is perfectly fine, but in that case, the errors should have the same code.

Since both these cases serve the same purpose, it might make sense to just have the same error and code in both cases.

so you're differentiating between conflicting key -> key and key -> value names

I did that because the error logic was at different places in the code and I tend to write more descriptive error msg if possible. But here I agree the solidity dev that ends up on this error doesn't benefit much.

nikola-matic · 2022-11-23T14:37:53Z

test/libsolidity/syntaxTests/parsing/mapping_with_names_struct_member_4.sol

@@ -0,0 +1,6 @@
+contract test {
+    struct Person {
+        mapping(uint phone => mapping(uint call => uint time) callTimes) friends;


Adding another test case with an additional level of nesting would be nice, especially if it hits the conflicting parameter error between the root and leaf mapping, e.g.

mapping(uint phone => mapping(uint call => mapping(uint phone => something) leafMapping) callTimes) friends;

There was a typo in the logic and this test case caught that. I've updated the fix in commit 7438248.

nikola-matic · 2022-11-23T20:33:07Z

libsolidity/analysis/DeclarationTypeChecker.cpp

+					m_errorReporter.declarationError(
+						1809_error,
+						_mapping.location(),
+						"Conflicting parameter name \"" + keyName + "\" in recursive mapping."


Oh yeah, one thing that kinda irks me is this being called a recursive mapping. Nested would be a more appropriate term here.

I've just combined the error into single one, removing the "recursive" word itself. So now it looks like: Conflicting parameter name "nameSame" in mapping.. Lmk if we have to change this into something better.

nikola-matic · 2022-11-24T16:21:40Z

I've pushed an empty commit, squash and pushed in the hopes of restarting the build successfully, but unfortunately we seem to have CI problems, which will hopefully be resolved soon. In fact, as soon as that happens, I'll restart the build to see whether everything passes, and then I'll resume reviewing.

libsolidity/analysis/DeclarationTypeChecker.cpp

nikola-matic · 2022-11-24T21:23:08Z

libsolidity/analysis/DeclarationTypeChecker.cpp

+				ASTString childKeyName = childMappingType->keyName();
+				if (keyName == childKeyName)
+				{
+					m_errorReporter.declarationError(


The chk_errorcodes step is failing because you can't throw an error with the same code twice, i.e. this one, and the one on line 310; instead you'll have to use a boolean to indicate whether an error needs to be thrown, and then check at the end of the loop iteration and report error if true.

I've just pushed a change for this.

Co-authored-by: Nikola Matić <nikola.matic@ethereum.org>

nikola-matic · 2022-11-25T08:07:16Z

libsolidity/analysis/DeclarationTypeChecker.cpp

+				if (keyName == currentValueName)
+				{
+					isError = true;
+					break;
+				}


Suggested change

if (keyName == currentValueName)

{

isError = true;

break;

}

if (keyName == currentValueName)

isError = true;

libsolidity/analysis/DeclarationTypeChecker.cpp

nikola-matic · 2022-11-25T08:14:56Z

test/libsolidity/syntaxTests/parsing/mapping_with_names_struct_member_6.sol

@@ -0,0 +1,7 @@
+contract test {
+    struct Person {
+        mapping(uint nameSame => mapping(uint name1 => mapping(uint nameSame => uint name2) name3) name4) name5;


Another three-level nested mapping test, but this time valid, please :)

I've added that, please review.

zemse dismissed stale reviews from ghost via 48df82f Aug 13, 2022

zemse commented Aug 13, 2022

View changes

cameel added the external contribution ⭐ label Aug 14, 2022

hrkrshnn reviewed Aug 17, 2022

View changes

test/libsolidity/syntaxTests/parsing/mapping_with_names_5.sol Show resolved Hide resolved

zemse dismissed a stale review via cf2056e Aug 17, 2022

zemse requested a review from hrkrshnn Aug 18, 2022

cameel requested a review from nishant-sachdeva Aug 18, 2022

Marenz suggested changes Aug 24, 2022

View changes

docs/types/mapping-types.rst Outdated Show resolved Hide resolved

libsolidity/parsing/Parser.cpp Show resolved Hide resolved

zemse force-pushed the develop branch from 140d685 to 50edfb0 Compare Aug 28, 2022

zemse force-pushed the develop branch from 50edfb0 to 303c830 Compare Sep 12, 2022

zemse requested review from Marenz and hrkrshnn and removed request for hrkrshnn, nishant-sachdeva and Marenz Sep 12, 2022

hrkrshnn force-pushed the develop branch from 303c830 to 694d3f0 Compare Sep 25, 2022

hrkrshnn reviewed Sep 25, 2022

View changes

Changelog.md Outdated Show resolved Hide resolved

hrkrshnn reviewed Sep 25, 2022

View changes

docs/types/mapping-types.rst Outdated Show resolved Hide resolved

hrkrshnn reviewed Sep 25, 2022

View changes

libsolidity/ast/AST.h Outdated Show resolved Hide resolved

hrkrshnn reviewed Sep 25, 2022

View changes

libsolidity/ast/ASTJsonImporter.cpp Outdated Show resolved Hide resolved

hrkrshnn reviewed Sep 25, 2022

View changes

docs/types/mapping-types.rst Outdated Show resolved Hide resolved

zemse and others added 6 commits Nov 20, 2022

test: add parser and abi test cases

07db7a1

docs: add example on using named parameters for mappings

c05147f

- Add changelog

feat: update antlr grammar to allow named parameters in mappings

e57a7b9

fix: prevent conflicting mapping parameter names

340799a

ref: change order of mapping initializers

8afe8fd

test: update expectations and fix build

6405728

zemse force-pushed the develop branch from 7844576 to 4aedd77 Compare Nov 20, 2022

test: add more tests

f4150d4

nikola-matic force-pushed the develop branch from 4aedd77 to f4150d4 Compare Nov 21, 2022

nikola-matic self-assigned this Nov 21, 2022

nikola-matic reviewed Nov 23, 2022

View changes

zemse added 3 commits Nov 24, 2022

fix: use common error & code for conflicting params

c03b6de

fix: issue with accessing nested mapping

7438248

test: add conflicting params tests for more nested levels

7142217

nikola-matic force-pushed the develop branch from 50bdbeb to 7142217 Compare Nov 24, 2022

nikola-matic reviewed Nov 24, 2022

View changes

libsolidity/analysis/DeclarationTypeChecker.cpp Outdated Show resolved Hide resolved

nikola-matic reviewed Nov 24, 2022

View changes

Update libsolidity/analysis/DeclarationTypeChecker.cpp

9070e2d

Co-authored-by: Nikola Matić <nikola.matic@ethereum.org>

nikola-matic force-pushed the develop branch from 825f3de to 67057e6 Compare Nov 25, 2022

nikola-matic reviewed Nov 25, 2022

View changes

libsolidity/analysis/DeclarationTypeChecker.cpp Outdated Show resolved Hide resolved

nikola-matic requested changes Nov 25, 2022

View changes

fix: error reported with the same code twice

4de394b

zemse force-pushed the develop branch from b608a4e to 02024df Compare Nov 26, 2022

test: add more tests for 3 level nested mapping

cf24f9d

zemse force-pushed the develop branch from 02024df to cf24f9d Compare Nov 26, 2022

Allow named parameters in mapping types #13384

Allow named parameters in mapping types #13384

zemse commented Aug 11, 2022 •

edited

zemse Aug 13, 2022

hrkrshnn left a comment

hrkrshnn commented Aug 17, 2022

hrkrshnn commented Aug 17, 2022

zemse commented Aug 18, 2022

Marenz left a comment

cameel commented Aug 24, 2022 •

edited

zemse commented Sep 12, 2022 •

edited

hrkrshnn commented Sep 25, 2022

hrkrshnn Sep 25, 2022

zemse commented Nov 20, 2022 •

edited

nikola-matic Nov 23, 2022 •

edited

zemse Nov 23, 2022

nikola-matic Nov 23, 2022

zemse Nov 24, 2022

nikola-matic Nov 23, 2022

zemse Nov 24, 2022

nikola-matic commented Nov 24, 2022

nikola-matic Nov 24, 2022

zemse Nov 25, 2022

nikola-matic Nov 25, 2022

nikola-matic Nov 25, 2022

zemse Nov 26, 2022

Allow named parameters in mapping types #13384

Are you sure you want to change the base?

Allow named parameters in mapping types #13384

Conversation

zemse commented Aug 11, 2022 • edited

Choose a reason for hiding this comment

hrkrshnn left a comment

hrkrshnn commented Aug 17, 2022

hrkrshnn commented Aug 17, 2022

zemse commented Aug 18, 2022

Marenz left a comment

cameel commented Aug 24, 2022 • edited

zemse commented Sep 12, 2022 • edited

hrkrshnn commented Sep 25, 2022

Choose a reason for hiding this comment

zemse commented Nov 20, 2022 • edited

nikola-matic Nov 23, 2022 • edited

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

Choose a reason for hiding this comment

nikola-matic commented Nov 24, 2022

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

zemse commented Aug 11, 2022 •

edited

cameel commented Aug 24, 2022 •

edited

zemse commented Sep 12, 2022 •

edited

zemse commented Nov 20, 2022 •

edited

nikola-matic Nov 23, 2022 •

edited