ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside :: Issue 11610 #12266

nishant-sachdeva · 2021-11-10T12:39:23Z

TypeChecker.cpp :: Replced typeError with fataTypeError in lines 390 and 406.
Test file added in test/libsolidity/syntaxTests/calldata_struct_argument_with_internal_type_inside.sol
Relevant line added to Changelog.md

Closes #11610

…06. Test file added in test/libsolidity/syntaxTests/calldata_struct_argument_with_internal_type_inside.sol

cameel

Congrats on the first PR!

I added some remarks for specific places in code in the comments below. Here are some general ones:

I'd avoid making PRs from develop branch in your fork. It just makes it harder to update it or make multiple PRs. Please use named branches instead.
Please use some sensible title for the PR (and a similar name for the branch). We generally don't know all issue numbers by heart so it's hard to say what it's about without looking it up :P In branch names the issue number also would make it annoying to type on the CLI because you have to type the number before you get autocompletion. Also no need to say "Draft" in the title.

cameel · 2021-11-10T12:48:22Z

Changelog.md

@@ -29,6 +29,7 @@ Bugfixes:
 * TypeChecker: Improved error message for constant variables with (nested) mapping types.
 * Yul Assembler: Fix internal error when function names are not unique.
 * Yul IR Generator: Do not output empty switches/if-bodies for empty contracts.
+ * ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside


Please rebase on latest develop. We have just relased 0.8.10 yesterday so this should be in the 0.8.11 section :)

cameel · 2021-11-10T12:49:07Z

test/libsolidity/syntaxTests/calldata_struct_argument_with_internal_type_inside.sol

+contract C {
+  struct S {
+    function() a;
+  }
+  function f(S[2] calldata) {}
+}


Suggested change

contract C {

struct S {

function() a;

}

function f(S[2] calldata) {}

}

contract C {

struct S {

function() a;

}

function f(S[2] calldata) {}

}

cameel · 2021-11-10T12:50:35Z

test/libsolidity/syntaxTests/calldata_struct_argument_with_internal_type_inside.sol

@@ -0,0 +1,9 @@
+contract C {


Unless the test is really unique in some way, I think it would be better to find a subdirectory where it fits best instead of dropping it directly into syntaxTests/.

I'd place it next to the test from #10516.

cameel · 2021-11-10T12:53:57Z

Changelog.md

@@ -29,6 +29,7 @@ Bugfixes:
 * TypeChecker: Improved error message for constant variables with (nested) mapping types.
 * Yul Assembler: Fix internal error when function names are not unique.
 * Yul IR Generator: Do not output empty switches/if-bodies for empty contracts.
+ * ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside


Does it happen for calldata structs or only for arrays of structs? Can you add a test case covering single structs (unless there's one already, please check)?

@cameel You've mentioned in #10516 that this works for a struct but not for an array of structs.
This is because isDynamicallyEncoded() does not get called in the first case case.

However, I've added a test case to check for a single struct. Will push in the next commit.

cameel · 2021-11-10T12:55:57Z

libsolidity/analysis/TypeChecker.cpp

@@ -387,7 +387,7 @@ bool TypeChecker::visit(FunctionDefinition const& _function)
 			_var.referenceLocation() == VariableDeclaration::Location::Storage &&
 			!m_currentContract->abstract()
 		)
-			m_errorReporter.typeError(
+			m_errorReporter.fatalTypeError(


This generally seems fine to me but please check if it shouldn't rather be fixed in the same place as #11000 instead.

cameel · 2021-11-10T13:26:42Z

About your question from the call: you need to update test expectations (isoltest can do it automatically), which will remove the error from some tests but after doing that please make sure that every test still makes sense. If stopping early makes it ineffective at testing what it was supposed to (i.e. it should produce a specific error message that no longer shows up), it may have to be adjusted.

chriseth · 2021-11-10T13:34:02Z

test/libsolidity/syntaxTests/calldata_struct_argument_with_internal_type_inside.sol

+  function f(S[2] calldata) {}
+}
+// ----
+// SyntaxError 4937: (50-78): No visibility specified. Did you intend to add "public"?


Please make the function public (function <functionName>() public ...) so that this error goes away - it is not relevant for the test.

Issue 11610 : Replced typeError with fataTypeError in lines 390 and 4…

4424288

…06. Test file added in test/libsolidity/syntaxTests/calldata_struct_argument_with_internal_type_inside.sol

nishant-sachdeva changed the title ~~Issue 11610 : Replced typeError with fataTypeError in lines 390 and 4…~~ Issue 11610 Nov 10, 2021

nishant-sachdeva changed the title ~~Issue 11610~~ Draft PR :: Issue 11610 Nov 10, 2021

cameel reviewed Nov 10, 2021

View reviewed changes

chriseth reviewed Nov 10, 2021

View reviewed changes

nishant-sachdeva changed the title ~~Draft PR :: Issue 11610~~ Draft PR :: Issue 11610 :: ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside Nov 11, 2021

nishant-sachdeva mentioned this pull request Nov 11, 2021

Calldata struct array arugment with internal type inside #12275

Merged

nishant-sachdeva closed this Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside :: Issue 11610 #12266

ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside :: Issue 11610 #12266

nishant-sachdeva commented Nov 10, 2021 •

edited

Loading

cameel left a comment •

edited

Loading

cameel Nov 10, 2021

cameel Nov 10, 2021

cameel Nov 10, 2021

cameel Nov 10, 2021

nishant-sachdeva Nov 11, 2021 •

edited

Loading

cameel Nov 10, 2021

cameel commented Nov 10, 2021

chriseth Nov 10, 2021

ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside :: Issue 11610 #12266

ICE in StructType::isDynamicallyEncoded() when a function has a calldata struct argument with an internal type inside :: Issue 11610 #12266

Conversation

nishant-sachdeva commented Nov 10, 2021 • edited Loading

cameel left a comment • edited Loading

Choose a reason for hiding this comment

cameel Nov 10, 2021

Choose a reason for hiding this comment

cameel Nov 10, 2021

Choose a reason for hiding this comment

cameel Nov 10, 2021

Choose a reason for hiding this comment

cameel Nov 10, 2021

Choose a reason for hiding this comment

nishant-sachdeva Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

cameel Nov 10, 2021

Choose a reason for hiding this comment

cameel commented Nov 10, 2021

chriseth Nov 10, 2021

Choose a reason for hiding this comment

nishant-sachdeva commented Nov 10, 2021 •

edited

Loading

cameel left a comment •

edited

Loading

nishant-sachdeva Nov 11, 2021 •

edited

Loading