Skip to content

Compiler stack: Protect loadGeneratedIR from contracts that cannot be deployed#15505

Merged
clonker merged 4 commits intodevelopfrom
protect_load_generated_ir
Oct 16, 2024
Merged

Compiler stack: Protect loadGeneratedIR from contracts that cannot be deployed#15505
clonker merged 4 commits intodevelopfrom
protect_load_generated_ir

Conversation

@clonker
Copy link
Member

@clonker clonker commented Oct 11, 2024

If a contract is not deployable (ie interface / abstract), no yulIR will be generated. This causes the object parser to error out in loadGeneratedIR, as it expects object {} as a bare minimum, but gets an empty string instead.

@clonker clonker requested a review from cameel October 11, 2024 13:10
@r0qs
Copy link
Member

r0qs commented Oct 11, 2024

I would also add a test case like:

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

abstract contract C {
	function f() public virtual returns (string memory);
}

and

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

interface I {
	function f() external returns (string memory);
}

Could be similar to our current ast ir command-line test (https://github.com/ethereum/solidity/blob/e8b5ca83f37b6a126db6a0a1dcfa00decff63647/test/cmdlineTests/ast_ir/args). And also one, or more, for standard Json output (https://github.com/ethereum/solidity/blob/e8b5ca83f37b6a126db6a0a1dcfa00decff63647/test/cmdlineTests/standard_ir_ast_requested/input.json):

{
	"language": "Solidity",
	"sources":
	{
		"C":
		{
			"content": "// SPDX-License-Identifier: GPL-3.0\npragma solidity >=0.0; abstract contract C {}"
		}
	},
	"settings": {
		"optimizer": {
			"enabled": true,
			"details": {
				"yul": true
			}
		},
		"viaIR": true,
		"outputSelection": {
			"*": {
				"*": ["irAst", "irOptimizedAst", "yulCFGJson"]
			}
		}
	}

}

Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Good catch. Looks like I unwittingly introduced this bug in the last release via #15451.

The fix is not wrong, but I'd do it differently in CompilerStack to make this kind of mistake harder to make in the future.

@clonker clonker force-pushed the protect_load_generated_ir branch from f2cd9a1 to 84e08d5 Compare October 14, 2024 08:10
r0qs
r0qs previously approved these changes Oct 14, 2024
Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

I have only one suggestion. But it looks good to me anyway :)

r0qs
r0qs previously approved these changes Oct 14, 2024
@ekpyron
Copy link
Collaborator

ekpyron commented Oct 14, 2024

This should probably also get a bugfix changelog entry. A bit annoying that this is in the release now, but I guess people rarely use these json artifacts anyways - might be a good indicator for how much they're used in the wild actually (since if they're used, people will quickly run into this I guess)

@clonker clonker force-pushed the protect_load_generated_ir branch 2 times, most recently from 3548b2d to 1f20ccd Compare October 14, 2024 16:53
@clonker clonker force-pushed the protect_load_generated_ir branch from 1f20ccd to a7eaac7 Compare October 15, 2024 07:02
@cameel cameel added the 🟡 PR review label label Oct 16, 2024
@clonker clonker force-pushed the protect_load_generated_ir branch from a7eaac7 to 7972c51 Compare October 16, 2024 07:05
@clonker clonker merged commit 3994386 into develop Oct 16, 2024
@clonker clonker deleted the protect_load_generated_ir branch October 16, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🟡 PR review label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants