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:53Z

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

libsolidity/analysis/DeclarationTypeChecker.cpp

libsolidity/ast/AST.h

libsolidity/ast/ASTJsonImporter.cpp

docs/types/mapping-types.rst

test/libsolidity/syntaxTests/parsing/mapping_with_names_conflict_3.sol

test/libsolidity/syntaxTests/parsing/mapping_with_names_struct_member_4.sol

libsolidity/analysis/DeclarationTypeChecker.cpp

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

test/libsolidity/syntaxTests/parsing/mapping_with_names_struct_member_6.sol

Unavailable

libsolidity/analysis/DeclarationTypeChecker.cpp

Co-authored-by: Hari <[email protected]> test: add parser and abi test cases docs: add example on using named parameters for mappings - Add changelog feat: update antlr grammar to allow named parameters in mappings fix: prevent conflicting mapping parameter names ref: change order of mapping initializers test: update expectations and fix build test: add more tests fix: use common error & code for conflicting params fix: issue with accessing nested mapping test: add conflicting params tests for more nested levels Update libsolidity/analysis/DeclarationTypeChecker.cpp Co-authored-by: Nikola Matić <[email protected]> fix: error reported with the same code twice test: add more tests for 3 level nested mapping Address review comments

nikola-matic

I've addressed the remaining issues, rebased and squashed, and am satisfied with the state of the PR at the moment, and am thus approving. @cameel or @ekpyron, please take over for the final review.

zemse dismissed stale reviews from ghost via 48df82f August 13, 2022 10:46

zemse commented Aug 13, 2022

View reviewed changes

cameel added the external contribution ⭐ label Aug 14, 2022

hrkrshnn reviewed Aug 17, 2022

View reviewed changes

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

zemse dismissed a stale review via cf2056e August 17, 2022 11:43

zemse requested a review from hrkrshnn August 18, 2022 05:33

cameel requested a review from nishant-sachdeva August 18, 2022 17:17

Marenz suggested changes Aug 24, 2022

View reviewed 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 August 28, 2022 07:52

zemse force-pushed the develop branch from 50edfb0 to 303c830 Compare September 12, 2022 18:15

zemse requested review from Marenz and hrkrshnn and removed request for hrkrshnn, nishant-sachdeva and Marenz September 12, 2022 18:21

hrkrshnn force-pushed the develop branch from 303c830 to 694d3f0 Compare September 25, 2022 20:18

hrkrshnn reviewed Sep 25, 2022

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

hrkrshnn reviewed Sep 25, 2022

View reviewed changes

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

hrkrshnn reviewed Sep 25, 2022

View reviewed changes

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

hrkrshnn reviewed Sep 25, 2022

View reviewed changes

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

hrkrshnn reviewed Sep 25, 2022

View reviewed changes

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

hrkrshnn reviewed Sep 25, 2022

View reviewed changes

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

nikola-matic self-assigned this Nov 21, 2022

nikola-matic reviewed Nov 23, 2022

View reviewed changes

test/libsolidity/syntaxTests/parsing/mapping_with_names_conflict_3.sol Outdated Show resolved Hide resolved

nikola-matic reviewed Nov 23, 2022

View reviewed changes

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

nikola-matic reviewed Nov 23, 2022

View reviewed changes

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

nikola-matic force-pushed the develop branch from 50bdbeb to 7142217 Compare November 24, 2022 16:19

nikola-matic reviewed Nov 24, 2022

View reviewed changes

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

nikola-matic reviewed Nov 24, 2022

View reviewed changes

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

nikola-matic force-pushed the develop branch from 825f3de to 67057e6 Compare November 25, 2022 06:32

nikola-matic reviewed Nov 25, 2022

View reviewed changes

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

nikola-matic reviewed Nov 25, 2022

View reviewed changes

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

nikola-matic requested changes Nov 25, 2022

View reviewed changes

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

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

zemse force-pushed the develop branch 2 times, most recently from 02024df to cf24f9d Compare November 26, 2022 07:02

nikola-matic force-pushed the develop branch from cf24f9d to 98deecf Compare December 6, 2022 15:48

nikola-matic reviewed Dec 6, 2022

View reviewed changes

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

nikola-matic reviewed Dec 6, 2022

View reviewed changes

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

nikola-matic force-pushed the develop branch from 98deecf to ae6502c Compare December 8, 2022 10:35

nikola-matic reviewed Dec 8, 2022

View reviewed changes

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

nikola-matic force-pushed the develop branch from ae6502c to fa78e0f Compare December 8, 2022 10:57

nikola-matic approved these changes Dec 8, 2022

View reviewed changes

NunoFilipeSantos requested a review from ekpyron December 13, 2022 11:35

ekpyron merged commit f441e13 into ethereum:develop Jan 9, 2023

ekpyron mentioned this pull request Jan 11, 2023

AST source locations for named mapping keys. #13864

Closed

SerCeMan mentioned this pull request Mar 4, 2023

fix #346: allow named parameters in mapping types intellij-solidity/intellij-solidity#347

Merged

cruzdanilo mentioned this pull request Jun 19, 2023

bump solidity version & use new features exactly/protocol#625

Open

zemse mentioned this pull request Feb 15, 2024

Include mapping parameter names in AST and StorageLayout #14856

Closed

OmarTawfik mentioned this pull request Jan 2, 2025

unbound named parameters in mapping types NomicFoundation/slang#1213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow named parameters in mapping types #13384

Allow named parameters in mapping types #13384

zemse commented Aug 11, 2022 •

edited

Loading

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

Loading

zemse commented Sep 12, 2022 •

edited

Loading

hrkrshnn commented Sep 25, 2022

nikola-matic commented Nov 24, 2022

nikola-matic left a comment

Allow named parameters in mapping types #13384

Allow named parameters in mapping types #13384

Conversation

zemse commented Aug 11, 2022 • edited Loading

zemse Aug 13, 2022

Choose a reason for hiding this comment

hrkrshnn left a comment

Choose a reason for hiding this comment

hrkrshnn commented Aug 17, 2022

hrkrshnn commented Aug 17, 2022

zemse commented Aug 18, 2022

Marenz left a comment

Choose a reason for hiding this comment

cameel commented Aug 24, 2022 • edited Loading

zemse commented Sep 12, 2022 • edited Loading

hrkrshnn commented Sep 25, 2022

nikola-matic commented Nov 24, 2022

nikola-matic left a comment

Choose a reason for hiding this comment

zemse commented Aug 11, 2022 •

edited

Loading

cameel commented Aug 24, 2022 •

edited

Loading

zemse commented Sep 12, 2022 •

edited

Loading