yul literal value as struct of u256 + optional formatting hint

[clonker]

Changed the definition of the solidity::yul::Literal to carry its value not as YulString but as LiteralValue struct, consisting of a u256 and an optional string representation hint. Upon converting from its data back to a string representation, it is first checked if the hint is not empty and in that case, whether value == parseValue(hint).

Wanted to comment and not approve.

[cameel]

Not a complete review yet. Halfway through reading it I started realizing that I thought I knew how the hint and the unlimited flag worked but I actually didn't. I'm more and more convinced that this flag is not exactly the right solution here and we need to change it a bit. It's not really a property of the literal itself. But see my comments for details.

And some style nitpicking :)

[cameel]

Can we assert here that the hint matches the data if present?

formatLiteral() ignores m_data when m_hint is present so we must ensure that these two are always consistent.

[cameel]

And I'd recommend this style:

Suggested change

	LiteralValue(Data const& _data, std::optional<std::string> const& _hint = std::nullopt, bool _unlimited = false):
	m_data(_data), m_hint(_hint ? std::make_shared<std::string>(*_hint) : nullptr), m_unlimited(_unlimited) {}
	LiteralValue(Data const& _data, std::optional<std::string> const& _hint = std::nullopt, bool _unlimited = false):
	m_data(_data),
	m_hint(_hint ? std::make_shared<std::string>(*_hint) : nullptr),
	m_unlimited(_unlimited)
	{}

[clonker]

Hmm asserting it in this place is going to be difficult without knowing the LiteralKind. That is what determines the validity in the end. I have added a check to formatLiteral in the utilities, though, so that the hint isn't blindly taken but it is compared to the stored u256 value.

Alternatively, we could think about storing the LiteralKind in the LiteralValue as well. Then an assert in the constructor is possible and failure would occur earlier.

[cameel]

Yeah, it really feels like LiteralKind should be inside LiteralValue, because the value is ambiguous without it. If would let us get rid of a lot of redundant asserts that we now have to do at every step to ensure the values are valid.

But I think it would require changing a lot more code so perhaps better to leave at is for now and just finish the PR in the current state.

[cameel]

This looks like you're formatting strings and integers the same way. How can that work? I.e. how does it preserve the distinction between 1234 and "1234"?

Actually, does the hint include the quotes? Because if it does not, then I can't see how we can distinguish values like '1234' and "1234" or '1234' and hex'1234'.

Also, if there's no distinction, the value cannot be unambiguously recovered from the hint (which matters if we want to enforce that they match).

[cameel]

By the way, it would be good to have those examples as test cases if we don't have them somewhere already.

I'd also add something like this:

object "A" {
    code {
        pop(datasize(hex'616263'))
    }
    data 'abc' "1234"
}

I actually only now realized that we're allowing anything other than plain strings for literal arguments, not sure we even have it covered (only for arguments though, not in data).

[clonker]

The quoting and escaping of string literals is done in AsmPrinter, so the LiteralKind is also always required to unambiguously print and/or recover a literal. In your example you'd get

object "A" {
    code {
        pop(datasize(hex'616263'))
    }
    data 'abc' "1234"
}
// ----
// step: disambiguator
//
// object "A" {
//     code { pop(datasize("abc")) }
//     data "abc" hex"31323334"
// }

as for almost ambiguous literals

{
    let a := 1234
    let a_hex := 0x4d2
    let b := "1234"
    let b_hex := "0x4d2"
    let c := '1234'
    let c_hex := '0x4d2'
    let d := hex"1234"
    let d_hex := hex"04d2"
}
// ----
// step: disambiguator
//
// {
//     let a := 1234
//     let a_hex := 0x4d2
//     let b := "1234"
//     let b_hex := "0x4d2"
//     let c := "1234"
//     let c_hex := "0x4d2"
//     let d := "\x124"
//     let d_hex := "\x04\xd2"
// }

[cameel]

Oh, so we're actually not preserving the representation perfectly? That's surprising. I had the impression that the AST allowed us to mostly reproduce the original source aside from comments and whitespace and that the representation hint was meant to help with that, but apparently not. I see from your output that it doesn't even keep hex numbers as hex so it seems kinda useless.

I wonder what we'd really lose by removing it. I wouldn't do it in this PR, but maybe we should consider that later?

[clonker]

Yeah, agree, this can probably be removed and hopefully make for a much cleaner data structure without losing too much

[cameel]

You're not validating the hint for boolean literals.

I see lots of checks for equality with "true" or "false" in the old code so I'm not sure why a lack of this check is not breaking anything. Are we missing test coverage?

[clonker]

The LiteralValue instances are only created via valueOfLiteral calls - that would take care of it already. But I agree, this should be checked!

[cameel]

formatLiteral() only has this single assert but I think there are more things you should have sanity checks against. For example that an unlimited literal is of the string kind or that the hint for boolean is actually true or false.

Or, if you define a validLiteral() function that executes the right validation based on the kind you could simply assert it at the beginning of the function. I've seen quite a few places where such a combined check would simplify assertions. AsmPrinter::operator()(Literal) is one example.

You might also use it at the end of valueOfLiteral() (or maybe better in each specific function, since they can be called individually). I see that you removed the original assertions from them but they seem still relevant to me. Or are the functions now used on invalid literals as well?

[clonker]

I was indeed using them on invalid literals so that the AsmAnalysis would produce proper errors for them instead of failing asserts: Having an invalid literal and then trying to format it into a string which then asserts validity is bound to go wrong.
But yeah, I have added a validLiteral method as you suggested and it does make the code cleaner :) for the AsmAnalysis reason I made it an optional (but default: on) assert in formatLiteral.

[cameel]

This compares value() between normal and unlimited strings. For syntactic equality, shouldn't we be comparing the string representations instead? I think that the previous version would have failed for different string representations of the same value and yours won't.

And value() comparison will never be true between a normal and unlimited string (assuming you the value of unlimited is nullopt). In fact an assert will fail if you run it on an unlimited string.

I wonder why none of this resulted in test failures. We're probably missing coverage here and if so, we should add a test that will trigger this code path.

[cameel]

Also, shouldn't invalid strings longer than 32 chars be syntactically equal to unlimited strings with the same representation? They will look identical in the source and have identical AST representation. And you already do consider two identical invalid strings to be equal so it would make sense to do the same thing for invalid compared with valid unlimited.

Can you even get an invalid string here? Maybe this should be an assert instead? Such a situation may only happen if you take an AST where a function call argument is a normal string on a position where an unlimited string is expected or the other way around. The parser after your changes won't ever produce anything like this so this could only happen if someone somehow imported doctored Yul AST with this exact node configuration - and we don't support Yul AST import right now. And if we did, I'd expect such AST to fail validation during import. We should probably just have a validLiteral() assert at the beginning of this function.

[clonker]

I am a bit confused about this one. There's a test like

{
  f()
  g()
  function f() { mstore(0x01, mload(0x00)) }
  function g() { mstore(1, mload(0)) }
}
// ----
// step: equivalentFunctionCombiner
//
// {
//     f()
//     f()
//     function f()
//     { mstore(0x01, mload(0x00)) }
//     function g()
//     { mstore(1, mload(0)) }
// }

which uses the SyntacticallyEqual under the hood. Now clearly f and g are not syntactically equal. If I change the implementation to compare the syntax (somewhat at least) by return formatLiteral(_lhs) == formatLiteral(_rhs);, this test will fail as the expectation is no longer met.

[clonker]

This component is not checking for strict syntactical equality but rather checks whether two things are semantically equal while evaluating syntactical equality - but doesn't really check full semantic equality either. :D I have added a statement in the header that literals are compared based on their value and not based on their string representation.

[ekpyron]

It's a really a syntax-based comparison that may only compare equal for things that are semantically equivalent :-). (While comparing unequal for semantically equivalent cases is not invalid, although the more semantically equivalent cases compare equal the better.)
Or more precisely it's syntactic comparison modulo renamings and literal formatting.

[cameel]

One small problem here is that if we ever add any extra checks to those validation functions, the messages will no longer match the errors.

We could make the messages more generic but maybe a better way to do this would be to keep the original checks and just assert validLiteral(_literal) after them to make sure that nothing slipped through?

[clonker]

Yeah that makes sense. I pulled them out into the utilities functions at some iteration of the PR because they became a bit lengthy but now it doesn't seem too bad.

[cameel]

I see that you're handling both normal and unlimited strings here even though here we know we're analyzing a call with unlimited literal arguments. Given that in the parser you made sure to use unlimited LiteralValue for such calls, it would be perfectly fine to just assert here that the argument is such a value and make the code simpler with that assumption.

In fact, if the parsing is wrong in some way and misses some cases, being too liberal here will let them slip through and we won't discover them easily.

[cameel]

BTW, isUnlimitedLiteralArgument is redundant anyway. It will always be true here because we're inside this overcomplicated condition that already checked that this is the case:

		if (
			auto literalArgumentKind = (literalArguments && i <= literalArguments->size()) ?
				literalArguments->at(i - 1) :
				std::nullopt
		)

[clonker]

nice, done that:)