Support "standardised" JSON compiler input/output by axic · Pull Request #1639 · argotorg/solidity

axic · 2017-02-02T22:53:52Z

Implements #1387. Replaces #712.

Fixes #579, #863, #1645, #1809.

Todo:

axic · 2017-03-16T14:04:49Z

Working on this next.

axic · 2017-03-16T15:38:14Z

Added (locally) the JS interface: compileStandard(<string>, <readCallback>) -> <string>.

Any better naming?

axic · 2017-03-17T13:51:00Z

To keep this PR simple, it will not change the CLI to use StandardCompiler internally. Should be handled in a following PR.

chriseth

Did not yet review all of it.

chriseth · 2017-03-23T10:48:17Z

libsolidity/interface/StandardCompiler.h

+{
+
+/**
+ * Easy to use and self-contained Solidity compiler with as few header dependencies as possible.


Needs to be updated.

chriseth · 2017-03-23T10:48:32Z

libsolidity/interface/StandardCompiler.h

+#include <boost/filesystem.hpp>
+#include <json/json.h>
+#include <libdevcore/Common.h>
+#include <libdevcore/FixedHash.h>


I think most of the headers can go, CompilerStack should suffice.

chriseth · 2017-03-23T10:48:56Z

libsolidity/interface/StandardCompiler.h

+	/// File reading callback.
+	using ReadFileCallback = std::function<ReadFileResult(std::string const&)>;
+
+	/// Creates a new compiler stack.


Needs to be updated.

axic · 2017-03-29T12:08:21Z

Added error reporting of JSON parsing using jsoncpp::getFormattedErrorMessages():

Standard Input JSON: {xxx "language": "Solidity", "sources": { "mortal": { "keccak256": "0x234...", "content": "contract owned { address owner; }x contract mortal is owned { function kill() { if (msg.sender == owner) selfdestruct(owner); } }" } }, "settings": { "remappings": [ ":g/dir" ], "optimizer": { "enabled": true, "runs": 500 }, "metadata": { "useLiteralContent": true }, "libraries": { "myFile.sol": { "MyLib": "0x123123" } }, "outputSelection": { "*": { "*": [ "metadata", "evm.bytecode" ], "": [ "ast", "why3" ] } } } }

Standard Output JSON: {"errors":[{"component":"general","message":"* Line 1, Column 2\n  Missing '}' or object member name\n","severity":"error","type":"JSONError"}]}

It looks a bit weird since it uses asterisk for the list, and maybe we could consider using the sourceLocation parameter for location within the JSON if the type is JSONError.

chriseth · 2017-04-06T14:39:20Z

libsolidity/interface/StandardCompiler.cpp

+using namespace dev::solidity;
+
+Json::Value formatError(
+	bool const& _warning,


"small" types are usually passed by value and not by const reference.

chriseth · 2017-04-06T14:41:57Z

libsolidity/interface/StandardCompiler.h

+class StandardCompiler: boost::noncopyable
+{
+public:
+	struct ReadFileResult


We have the same in CompilerStack.h, right? Can you pull both out into another file? (Likewise with ReadFileCallback)

chriseth

Still not finished reviewing everything.

chriseth · 2017-04-06T14:45:56Z

libsolidity/interface/StandardCompiler.h

+	/// @param _readFile callback to used to read files for import statements. Should return
+	StandardCompiler(ReadFileCallback const& _readFile = ReadFileCallback())
+	{
+		CompilerStack::ReadFileCallback fileReader = [this,_readFile](std::string const& _path)


Is the only thing that this lambda does converting between this ReadFileResult and CompilerStack::ReadFileResult?

Yes. Though I wanted to change separate the string into error / result, but that is not so important.

Basically we'd need to pull out struct ReadFileResult {} into its own header which feels rather slim.

Added in #2111.

chriseth · 2017-04-06T14:47:02Z

libsolidity/interface/StandardCompiler.h

+			return CompilerStack::ReadFileResult{ret.success, ret.contentsOrErrorMessage};
+		};
+
+		m_compilerStack.reset(new CompilerStack(fileReader));


If you can get rid of the lambda, you can use

StandardCompiler(ReadFileCallback const& _readFile = ReadFileCallback()) : m_compilerStack(_readFile) { }

chriseth · 2017-04-06T14:47:36Z

libsolidity/interface/StandardCompiler.h

+		m_compilerStack.reset(new CompilerStack(fileReader));
+	}
+
+	/// Sets all input parameters according to @a _input which conforms to the standardized input


And performs compilation and returns the result.

chriseth · 2017-04-06T14:48:23Z

libsolidity/interface/StandardCompiler.h

+	/// Sets all input parameters according to @a _input which conforms to the standardized input
+	/// format.
+	Json::Value compile(Json::Value const& _input);
+	std::string compile(std::string const& _input);


Please document: Parses input as json, compiles and formats output json as string. Json decoding errors are reported in the same way as regular compiler errors.

chriseth · 2017-04-06T14:51:37Z

libsolidity/interface/StandardCompiler.cpp

+using namespace dev;
+using namespace dev::solidity;
+
+Json::Value formatError(


For added C++ishness, you can move those functions into an anonymous namespace:

namespace { ... functions... }

It makes them translation-unit local (similar to static in C).

Thanks, will add those.

chriseth · 2017-04-06T15:08:17Z

libsolidity/interface/StandardCompiler.cpp

+	Json::Value input;
+	Json::Reader reader;
+
+	try {


Braces on the next line.

chriseth · 2017-04-06T15:08:25Z

libsolidity/interface/StandardCompiler.cpp

+
+	try {
+		if (!reader.parse(_input, input, false))
+		{


No braces for single statement.

chriseth · 2017-04-06T15:08:51Z

libsolidity/interface/StandardCompiler.cpp

+	}
+	catch(...)
+	{
+		return "{\"errors\":\"[{\"type\":\"JSONError\",\"component\":\"general\",\"severity\":\"error\",\"message\":\"Error parsing input JSON.\"}]}";


Can you also use a utility function for this? Or do you want to avoid using the json formatter?

I have a hardcoded text in two places to avoid endless exception catching of jsonCompactPrint.

chriseth · 2017-04-06T15:27:15Z

libsolidity/interface/StandardCompiler.cpp

+	for (auto const& ref: linkReferences)
+	{
+		string const& fullname = ref.second;
+		size_t colon = fullname.find(':');


Please assert that colon != string::npos

chriseth · 2017-04-06T15:28:30Z

libsolidity/interface/StandardCompiler.cpp

+		string const& name = fullname.substr(colon + 1);
+
+		Json::Value fileObject = ret.get(file, Json::objectValue);
+		Json::Value libraryArray = fileObject.get(name, Json::arrayValue);


I think if you use Json::Value& libraryArray here, you do not need the assignment in line 169

This didn't yet work, would be nice to avoid the current way.

chriseth

I'm through!

chriseth · 2017-04-07T11:10:38Z

solc/CommandLineInterface.cpp

 		)
-		(g_argMetadataLiteral.c_str(), "Store referenced sources are literal data in the metadata output.");
+		(g_argMetadataLiteral.c_str(), "Store referenced sources are literal data in the metadata output.")
+		(g_argStandardJSON.c_str(), "Process standard JSON input / output.");


Process seems a bit too technical for me. Also, this disables some other options, does it? Perhaps we should make it more prominent (i.e. put it in the firstline) and say Switches the compiler into JSON input/output mode.

What is the "JSON input/output mode"?

Shouldn't it be irrelevant to the user how it operates internally? Ideally the CLI should go through the JSON IO to reduce the number of APIs we have, but that shouldn't have any effect on the user.

The current --standard-json is only an interface to directly interact with the API.

I thought the idea was to expose the json/io API on all implementations of the compiler, in order to get a unified interface and to avoid the problems we currently have with the filesystem.

I am a bit confused, but do not really see a point of having two implementations internally in the CLI, one using CompilerStack and the other using StandardCompiler.

I would just change it to use StandardCompiler only.

The above option though is not for switching between these implementation, but to accept a raw JSON input and output a raw JSON output.

chriseth · 2017-04-07T11:14:04Z

solc/CommandLineInterface.cpp

+	if (m_args.count(g_argStandardJSON))
+	{
+		string input;
+		while (!cin.eof())


There should be something in CommonIO.h to do exactly this.

Or do you want to convert \r\n to \n? I don't think this is a good idea.

There's none in CommonIO. If there's a sample code somewhere I'm happy to use that. This came from CommandLineInterface::readInputFilesAndConfigureRemappings.

(2) istream& getline (istream& is, string& str);

Extracts characters from is and stores them into str until the delimitation character delim is found (or the newline character, '\n', for (2)).

According to this, it won't cut off \r? And the \n is pushed back:

string input; while (!cin.eof()) { string tmp; getline(cin, tmp); input.append(tmp + "\n"); }

You are right, my mistake, there are only functions for files. cin is not opened in binary mode, so I think it will convert \r\n into \n. But actually, this should be irrelevant, since everything is json-encoded and whitespace should be just ignored anyway. So, yes, just leave it like that!

chriseth · 2017-04-07T11:16:43Z

solc/CommandLineInterface.cpp

+		string output = compiler.compile(input);
+		cout << "Standard Input JSON: " << input << endl << endl;
+		cout << "Standard Output JSON: " << output << endl;
+		return false;


The CLI is split into processInput and actOnInput. I'm not sure if this division makes sense here, but I think we should only run the compile step in the actOnInput function.

Oh and perhaps we should flag an error if --standard-json is combined with anything else.

It turns out that distinction of processInput vs. actOnInput is already broken by assemble and link in processInput. In that case it makes sense there. I would say at this point of time probably the distinction is not useful, because exception catching is handled mostly in actOnInput anyway.

chriseth · 2017-04-07T11:18:21Z

solc/jsonCompiler.cpp

+	{
+		readCallback = [=](string const& _path)
+		{
+			char* contents_c = nullptr;


This code should be duplicated above. Can you reuse it?

pipermerriam · 2017-04-20T22:51:39Z

Woooohooo!!!! 🍰 !

axic added in progress labels Feb 2, 2017

axic mentioned this pull request Feb 2, 2017

Work in Progress: Use standardized compiler input. #712

Closed

axic changed the title ~~WIP: Support "standardised" JSON compiler input/output~~ [WIP] Support "standardised" JSON compiler input/output Feb 6, 2017

axic force-pushed the json-interface-api branch 3 times, most recently from eda1ab2 to b0ed329 Compare February 14, 2017 17:12

axic force-pushed the json-interface-api branch 4 times, most recently from af166fa to a7502ff Compare March 16, 2017 13:30

axic self-assigned this Mar 16, 2017

axic force-pushed the json-interface-api branch 2 times, most recently from a16d202 to 6f07bd0 Compare March 17, 2017 12:30

chriseth suggested changes Mar 23, 2017

View reviewed changes

axic force-pushed the json-interface-api branch 6 times, most recently from 38d500c to 9d449df Compare March 30, 2017 01:10

axic mentioned this pull request Mar 31, 2017

Impossible to differentiate link references in bytecode #1645

Closed

chriseth reviewed Apr 6, 2017

View reviewed changes

chriseth suggested changes Apr 6, 2017

View reviewed changes

chriseth suggested changes Apr 7, 2017

View reviewed changes

axic added 23 commits April 20, 2017 19:38

Support the AST output

5aec120

Fail if no sources are given

2c15e2b

Refactor formatError

b3c6b1e

Support proper error reporting in StandardCompiler

f25efce

Support methodIdentifiers

63b6d00

Move opcodes inside the bytecode section

a87dd17

Support new assembly output

2df52ed

Support linkReferences

ad1fe86

Properly split contract filename and name

ef559de

Support the metadata.useLiteralContent setting

df7e703

Reject import URLs for now

77d5148

Support gas estimates in StandardCompiler

b513db7

Support Why3 in StandardCompiler

aa0776d

Enclose local functions in a namespace

413c146

Pull out collectEVMObject

6d13260

Ensure the language field is present in the JSON

f71b465

Add basic tests for StandardCompiler

10ec334

Trigger cases by the JSON (and not string) interface

137ef78

Add compilation test and verify output

a24ac2f

Rename ast to legacyAST in StandardCompiler

6ae7a87

Update changelog

5732ffd

Add test for bytecode

c29da75

Add test for legacyAST

74373ec

axic merged commit 2ccbc08 into develop Apr 20, 2017

axic removed the in progress label Apr 20, 2017

axic deleted the json-interface-api branch April 20, 2017 19:12

axic mentioned this pull request Apr 20, 2017

Not able to link a lib if you have >= 36 symbols in the path #1809

Closed

axic mentioned this pull request Apr 21, 2017

Export unlinked objects via json output #863

Closed

cag mentioned this pull request May 1, 2017

Testing issues ethereum/populus#287

Closed

Conversation

axic commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Mar 16, 2017

Uh oh!

axic commented Mar 16, 2017

Uh oh!

axic commented Mar 17, 2017

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

axic commented Feb 2, 2017 •

edited

Loading

chriseth Apr 6, 2017 •

edited

Loading

axic Apr 10, 2017 •

edited

Loading