Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed the message of the error thrown when the structure has all it's members omitted #12361

Conversation

mejsiej
Copy link
Contributor

@mejsiej mejsiej commented Dec 2, 2021

Closes #12302

@mejsiej mejsiej marked this pull request as draft December 2, 2021 11:39
@mejsiej mejsiej marked this pull request as ready for review December 3, 2021 08:54
cameel
cameel previously requested changes Dec 8, 2021
libsolidity/analysis/TypeChecker.cpp Outdated Show resolved Hide resolved
libsolidity/analysis/TypeChecker.cpp Outdated Show resolved Hide resolved
m_errorReporter.typeError(6744_error, _variable.location(), "Internal or recursive type is not allowed for public state variables.");
{
if (getter.returnParameterNames().empty() && getter.returnParameterTypes().empty() && getter.parameterNames().empty() && getter.parameterTypes().empty())
m_errorReporter.typeError(5359_error, _variable.location(), "The structure has all it's members omitted, therefore the getter cannot return any values.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this message changes the expected output in some syntax tests. Please run isoltest with the --update option locally and commit the changes.

Also note that the chk_errorcodes is failing because it ensures that we have at least one case covering each error. There's no test for the newly added error 5359. Running isoltest will probably change that but it seems we only have one test case for such getters and I think we need more to test this properly. Could you add some more cases, covering various situations that can result in such a getter?

  • Mapping in the struct.
  • Dynamic arrays other than bytes/string in the struct.
  • Multiple fields being omitted (rather than just one).
  • Struct containing a struct that has all its fields omitted (see Getters returning structs can not be defined in interfaces #11826 (comment)).
    struct T { mapping(uint => uint) m; }
    struct S { T t; }
    or
    struct U { mapping(uint => uint) m; }
    struct T { U u; }
    struct S { T t; }
  • A struct that contains two fields: one that won't be omitted (e.g. uint) and another that is also a struct and would have all of its members omitted.
    struct U { mapping(uint => uint) m; }
    struct T { U u; }
    struct S { uint i; T t; }
  • Nested dynamic array. Surprisingly, this does not fail. Apparently the rule for dynamic arrays is not recursive.
    struct T { uint[] u; }
    struct S { T t; }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can try.

@chriseth chriseth force-pushed the improved-err-msg-when-getter-returns-nothing branch from cebb86a to ddd9a84 Compare December 20, 2021 17:17
Copy link
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks a lot!

@chriseth chriseth enabled auto-merge December 20, 2021 17:27
@cameel cameel dismissed their stale review December 20, 2021 20:06

Would be nice to get the extra tests because we have very little coverage for this but I think the PR would still pass those tests.

@chriseth chriseth merged commit a4b2fc9 into ethereum:develop Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error message when a getter would return zero values and the compiler refuses to generate it
3 participants