[djolertrk]

Defines JSON_USE_IMPLICIT_CONVERSIONS=0 for the whole build. Converting a JSON value to a C++ type can fail, so it should be visible at the call site as an explicit get<T>().

The few implicit conversions this turned up are made explicit:

• test/libevmasm/Assembler.cpp and test/libsolidity/StandardCompiler.cpp: five std::string initializations from JSON values.
• StandardCompiler::parseInput(): the line ret.sources = Json::object(); assigned an empty JSON object to the std::map of a freshly constructed struct and did nothing, so it is removed.

The macro only makes JSON-to-C++ conversions explicit. Conversions from C++ types to JSON through to_json() remain implicit; nlohmann has no switch for those.

Suggested in #16990 (comment)...

[djolertrk]

cc @cameel

[blishko]

Is this uncovering some problem in emscripten? Looking at the CI failure of bytecode_compare job.

[djolertrk]

Is this uncovering some problem in emscripten? Looking at the CI failure of bytecode_compare job.

well, it is not an emscripten problem, but I would say a real one uncovered by this change. The ABI entries are sorted by comparing tuples of JSON values, and tuples use operator<=>. Emscripten’s standard library has no three-way comparison, so nlohmann::json doesn’t define operator<=> there. Once implicit conversions are disabled, a <=> b resolves through the remaining implicit operator value_t() and compares only the JSON types, so the ABI was left unsorted. The native builds have a real operator<=> and aren’t affected. I changed the comparator to compare the type and name as strings, which gives the same order everywhere.

[blishko]

Interesting, thanks for analyzing the problem!
I would still like to understand it more. Do you think this would be solved also by updating emscripten version, with newer standard library?
Would your fix be desirable anyway?

[clonker]

Do you think this would be solved also by updating emscripten version, with newer standard library?

yes, emscripten >= 4.0.0 should do it. it comes with llvm 19.1.6. which defines __cpp_lib_three_way_comparison.

[djolertrk]

Do you think this would be solved also by updating emscripten version, with newer standard library?

yes, emscripten >= 4.0.0 should do it. it comes with llvm 19.1.6. which defines __cpp_lib_three_way_comparison.

hmm it seems right, let me double check, thank you for the comment

[djolertrk]

Do you think this would be solved also by updating emscripten version, with newer standard library?

yes, emscripten >= 4.0.0 should do it. it comes with llvm 19.1.6. which defines __cpp_lib_three_way_comparison.

hmm it seems right, let me double check, thank you for the comment

But, I’d still keep the fix, though. The old comparator only worked by accident of overload resolution, with implicit conversions on, a <=> b was ambiguous, so the tuple fell back to operator<. Whether it sorts correctly depends on standard library features, and when it doesn’t, nothing fails to compile and the output is just quietly wrong. Comparing the type and name as strings says what we mean and doesn’t depend on any of that. The order is the same as before in every configuration I checked.

Bumping emscripten seems worthwhile on its own, but I’d do it separately from this PR. wdyt?

[blishko]

But, I’d still keep the fix, though.

I agree now.

Bumping emscripten seems worthwhile on its own, but I’d do it separately from this PR. wdyt?

Oh, definitely. Updating emscripten is a quest on its own. I have been working through the versions slowly, it's on pause at the moment.

[blishko]

I now support this change.
However, I am not convinced we need the extra comment. Opinions?

[blishko]

I am not sure we need to mention the three-way comparison and its (lack of) support in nlohmann::json.
Such a comment can get outdated quickly.
Also, I am not sure what keys you mean here.

I am not sure we need a comment at all.
If anything, I would personally prefer the following:

// ABI entries are sorted based on their type and name.

[clonker]

I'd drop the comment

[djolertrk]

yeah, I agree

[clonker]

There is another site with the three-way comparison here

solidity/libsolidity/ast/ASTJsonExporter.cpp

Line 677 in bbfadd4

std::sort(externalReferences.begin(), externalReferences.end());

I think the better fix is to set JSON_HAS_THREE_WAY_COMPARISON=1 as compile flag and then update emscripten soon. I am fairly confident these are the only two sites but yeah, it's quite brittle to say the least. Any ordered container, sorting, and/or general comparison between json objects is potentially broken right now.

The test macro __cpp_lib_three_way_comparison that nlohmann json relies on is not set because of unrelated features not being ready yet (like chrono), the spaceship operator itself is available. To be extra safe we can lift the runtime fail to a compile time error:

static_assert(
    std::same_as<decltype(std::declval<Json const&>() <=>
    std::declval<Json const&>()), std::partial_ordering>
);

With emscripten >= 4.0.0 we can remove the define again if we agree to add it here.

[djolertrk]

There is another site with the three-way comparison here

solidity/libsolidity/ast/ASTJsonExporter.cpp

Line 677 in bbfadd4

std::sort(externalReferences.begin(), externalReferences.end());

I think the better fix is to set JSON_HAS_THREE_WAY_COMPARISON=1 as compile flag and then update emscripten soon. I am fairly confident these are the only two sites but yeah, it's quite brittle to say the least. Any ordered container, sorting, and/or general comparison between json objects is potentially broken right now.

The test macro __cpp_lib_three_way_comparison that nlohmann json relies on is not set because of unrelated features not being ready yet (like chrono), the spaceship operator itself is available. To be extra safe we can lift the runtime fail to a compile time error:

static_assert(
    std::same_as<decltype(std::declval<Json const&>() <=>
    std::declval<Json const&>()), std::partial_ordering>
);

With emscripten >= 4.0.0 we can remove the define again if we agree to add it here.

Well, that’s a better fix. So this is addressed:

• JSON_HAS_THREE_WAY_COMPARISON=1 is set next to JSON_USE_IMPLICIT_CONVERSIONS=0, with a TODO to remove it once we’re on emscripten >= 4.0.0.
• The static_assert @clonker shared is in libsolutil/JSON.h, right after the Json alias. I checked it locally and it passes with the define and fails to compile without it. With implicit conversions on, it fails on the ambiguous <=>, and with them off, on the strong_ordering from value_t. That covers the ASTJsonExporter sort and any other ordered comparison of JSON values.
• The ABI comparator change is reverted, since it isn’t needed anymore.