Output selection is not validated in Standard JSON

[cameel]

Description

The compiler does not validate the values specified in the output type array inside settings.outputSelection in Standard JSON. It does not report an error if you specify an output that does not exist. This makes it harder to notice a problem if you misspell it or e.g. use something like evm.metadata instead of metadata.

Steps to Reproduce

{
    "language": "Solidity",
    "sources": {"contract.sol": {"content": "contract C {}"}},
    "settings": {
        "outputSelection": {"*": { "*": ["xyz", "evm.metadata", "ZZZZZZZZZZZZZZZZZZZZZZZZZZ", ""]}}
    }
}

This compiles without errors even though none of the specified inputs exists.

NOTE: This is about validating the array (i.e. the ["xyz", "evm.metadata", "ZZZZZZZZZZZZZZZZZZZZZZZZZZ", ""] part in the above), not file or contract names.

[akali]

Hi! I would like to take this issue

[haltman-at]

I want to actually suggest not doing this. Or at least, adding a way to turn off such a thing. (This is obviously based on my point of view of working at Truffle and having worked on the code that interfaces with the compiler.)

The fact that the Solidity compiler doesn't validate outputSelection makes it really easy to write code that works with multiple versions at once. You know the particular output you want, you ask for it, and if you don't get it due to an old version, well, you don't get it due to an old version.

Let's suppose instead that asking for an unknown form of output caused an error -- the Vyper compiler does this, and, well, it's a pain! It means, when compiling Vyper, we have to check the compiler version, look up what particular forms of output that version supports, and make sure to only ask for those particular things.

Every single form of output that Solidity adds after this change is made would have to be accompanied by an appropriate version check. Right now, our code for interfacing with the Solidity compiler actually doesn't know about the version, anywhere! So if 0.8.10 adds output selection validation, and later 0.9.0 adds a new form of output that we want to get, well... it's going to make a mess.

Now the advantages of making this change are obvious; most people really don't want silent failure on bad outputSelection, we're probably a little unusual in actually wanting that. But, well, that is something we rely on, so I have to point out that this change does have substantial disadvantages as well. Maybe as a compromise there could be some sort of option one could pass somewhere to indicate, hey, don't validate the output selection, or something...? (Note that the option would have to be passed somewhere such that older versions would ignore it, rather than erroring due to an unexpected option.)

[cameel]

Honestly I'm a bit surprised that other options in Standard JSON do not already cause this problem but maybe that's because a lot of them are dictionaries and there stray keys might not be always rejected?

Anyway, this is not a high priority change, just something that has bitten me when I was testing stuff and seemed pretty straightforward to fix. I thought adding these validations would be a clear improvement. If that would be a problem for you though, then we need to consider it more carefully. I'll add it to the design backlog so that we can discuss it in the team.

A solution with a flag that disables stricter validation behavior on demand sounds fine to me. If such a flag must be backwards-compatible then maybe we could repurpose --ignore-missing for this. Currently it makes the compiler not complain about missing input files and skip them instead, which is logically close to what this new flag would do, just with JSON arrays. The name is even generic enough not to be confusing. The only problem is that it would only work on the CLI and we would need something else for solc-js - perhaps a new parameter to compile() and I'm not sure if that would be backwards-compatible.

[haltman-at]

Honestly I'm a bit surprised that other options in Standard JSON do not already cause this problem but maybe that's because a lot of them are dictionaries and there stray keys might not be always rejected?

Good question. :) The actual answer is there output selection is the only setting that we routinely set ourselves! The other settings are left to the user, so, if they put in settings that the particular version they're using doesn't recognize... well, that's not our fault. :)

(In truffle test, we do in certain conditions set settings.debug.revertStrings, and we do have to put a version check around that, which is annoying. (And we might want to look on turning on further debug settings once 0.8.10 comes out, which would entail a further version check...) But that's a separate area of the code, not part of the code that interfaces directly with the compiler, so it can do those version checks; it's actually putting the setting into a Truffle config rather than directly into the standard JSON. :P )

A solution with a flag that disables stricter validation behavior on demand sounds fine to me. If such a flag must be backwards-compatible then maybe we could repurpose --ignore-missing for this. Currently it makes the compiler not complain about missing input files and skip them instead, which is logically close to what this new flag would do, just with JSON arrays. The name is even generic enough not to be confusing. The only problem is that it would only work on the CLI and we would need something else for solc-js - perhaps a new parameter to compile() and I'm not sure if that would be backwards-compatible.

So, this suggestion may be on the hacky side, but if you could smuggle the extra setting into outputSelection itself, then it would surely be backward compatible. :)

[cameel]

So, this suggestion may be on the hacky side, but if you could smuggle the extra setting into outputSelection itself, then it would surely be backward compatible. :)

That's an interesting idea. It is a bit hacky but not overly so. We already support some special values like *. So e.g. we could make the compiler disable validation if you include ? as one of the items. Or maybe something like _ignore-invalid-values if we want it to be more self-explanatory.

Another idea would be to allow appending/prepending ? to values to make the compiler skip them if they are invalid. Unfortunately this would not work with older compilers (they would ignore them when invalid but you would not get the output if they're valid) so yours sounds more viable.

[haltman-at]

If you want to avoid too much awkward naming and special-casing, it could perhaps be a fake form of output, rather than a fake source or contract? And then it would presumably be specific to the specific area where it's used, but, I don't think that's a big problem.

[cameel]

Yeah, that was exactly what I had in mind. Adding it at a source/contract level would be problematic because source unit names have literally zero restrictions. Contact names have more but putting it as one of the outputs seems safer to me.

[haltman-at]

Oh OK, cool!

[Marenz]

and if you don't get it due to an old version, well, you don't get it due to an old version.

Couldn't you keep that approach and just ignore/accept/display the error? You'd have the same behavior?

[chriseth]

I skipped all the comments above, but I think we should only validate keys that are actual static keys and not things that are contract or file names. If you request something that does not exist then it will not end up in the output and you will notice it anyway....

[cameel]

@chriseth

but I think we should only validate keys that are actual static keys and not things that are contract or file names.

Right, this issue is only about validating the array of output types, i.e. the part that contains values like evm.bytecode or metadata. These are completely static for a given compiler. They can change between versions though.

To summarize my discussion with @haltman-at above:

• Using ["xyz", "evm.metadata", "ZZZZZZZZZZZZZZZZZZZZZZZZZZ", ""] would be a validation error.
• There would be a special value you could insert into the array to make the compiler not validate it. For example ? so that ["?", "xyz", "evm.metadata", "ZZZZZZZZZZZZZZZZZZZZZZZZZZ", ""] would not be a validation error.

@Marenz

Couldn't you keep that approach and just ignore/accept/display the error? You'd have the same behavior?

Not sure what you mean. If the compiler reports an error the compilation stops and ignoring it leaves Truffle without a compiled contract.

[Marenz]

it leaves Truffle without a compiled contract.

ah, you still get a contract when the output is ignored?

[Marenz]

A middle way could be to have a warning?

[chriseth]

This kind of sounds like it would create more trouble than it would solve problems...

[cameel]

ah, you still get a contract when the output is ignored?

Yeah, what you are ignoring is just a part of the output, which possibly is just not supported by your version of the compiler. E.g. you want generatedSources but your compiler is too old.

I think a warning for this would be annoying and user could not do anything about it.

This kind of sounds like it would create more trouble than it would solve problems...

The validation or the warning?

When working with Standard JSON I had several situations where I thought I was just not getting any output but in fact I was just using the wrong name. Not validating stuff might be useful for tools if they request it but I think that by default validations should be there.

[haltman-at]

it leaves Truffle without a compiled contract.

ah, you still get a contract when the output is ignored?

Right, exactly -- if solc errors, we get nothing; if it silently ignores nonexistent output selection, then we can still get bytecode, ABI, source maps, etc, just maybe we don't get generated sources or immutable references or such.

[cameel]

We discussed this today and opinions were split. Not everyone agrees that validating this is a good thing and also this is a very low priority thing so I'm moving this to the Icebox. It probably won't be implemented unless we get some actual demand for these validations.

[cameel]

Since there's no consensus about implementing it, it does not even qualify as nice to have so I guess I'm going to close it.