Added -Xi to include new fields in JSON output by marler8997 · Pull Request #7838 · dlang/dmd

marler8997 · 2018-02-04T16:55:18Z

Adds a new option (hidden for now) that allows an application to select what it wants in the JSON output, i.e.

dmd -Xi=compilerInfo -Xi=semantics

This would produce a JSON file looking like this:

{
    "compilerInfo" : {
        ...
    },
    "semantics": {
        ...
    }
}

The option is hidden because the only users of the option (currently known) will be rdmd and dub.

NOTE: the -probe equivalent would be:

dmd -Xf=- -Xi=compilerInfo

** json output now works even with no input files

dlang-bot · 2018-02-04T16:55:19Z

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18367	normal	dmd should not segfault on -X with libraries, but no source files

marler8997 · 2018-02-04T17:08:23Z

NOTE: I also plan on adding the extra fields that were added in (#7521), however, I thought that should be it's own set of changes.

Hopefully we can get this one merged quickly so I can add the new fields from -probe with a good amount of time before the next release.

wilzbach

Nice. That looks a lot better than your previous approach.
I'm fully in favor of this PR and hope we can get it as soon as possible ;-)

wilzbach · 2018-02-04T17:31:34Z

src/dmd/json.d

+    }
+
+    /**
+    Returns true if `c` is a valid character for the start of a name..


timotheecour · 2018-02-04T20:34:28Z

src/dmd/json.d

-    json.arrayEnd();
-    json.removeComma();
+
+    if (global.params.jsonQuery is null)


could something like this work? it would be cleaner (less special cases)

if global.params.jsonQuery is null global.params.jsonQuery = "modules"; // or whatever we want default query to be; maybe put all the queries here # then the rest can assume we always have `jsonQuery`

can the default (with no jsonQuery) output the array of modules as nested under a root json object instead (as done with jsonQuery) instead of at top-level, or would that be a breaking change we want to avoid? It kind of sucks to have 2 different formats (one with top-level array and one where it's nested)

Its definitely better to not break backwards compatibility. If at some point in the future someone determines it's OK in this case, then it's simple to remove the special case.

timotheecour · 2018-02-04T20:35:48Z

src/dmd/json.d

+
+        json.objectStart();
+        auto parser = QueryParser(global.params.jsonQuery);
+        for (;;)


while(true) seems more idiomatic?

FYI: for (;;) is the "idiomatic" way of Phobos. We should probably put such trivias in the style guide, s.t. they don't constantly pop up...

timotheecour · 2018-02-04T20:37:18Z

src/dmd/json.d

+            {
+                //json.generateError(format("invalid query: expected name but got '%s' (0x%02x)",
+                //    *parser.next, cast(ubyte)*parser.next));
+                json.generateError("invalid query: expected name or EOF but got something else".ptr);


most useful diagnostic would be a to show byte offset from start of string where parser got confused

(also this is uncovered)

workin on this one

ok, I ended up using vsnprintf, also added tests.

wilzbach · 2018-02-04T20:43:14Z

src/dmd/mars.d

+        if (global.params.jsonQuery)
+        {
+            generateJson(null);
+            return EXIT_SUCCESS;


Ah nice. I didn't see this initially.

This is more or less what I am trying to achieve in #7840

wilzbach · 2018-02-04T20:48:11Z

src/dmd/mars.d

+    {
+        // Write to stdout; assume it succeeds
+        size_t n = fwrite(buf.data, 1, buf.offset, stdout);
+        assert(n == buf.offset); // keep gcc happy about return values


Needs a test.
Example:

#!/usr/bin/env bash set -ueo pipefail name="$(basename "$0" .sh)" dir="${RESULTS_DIR}/compilable/" out="$dir/${name}.json.out" "$DMD" -X | ${RESULTS_DIR}/sanitize_json > "$out" diff "$out" compilable/extra-files/$name.json rm "$out" "$DMD" -Xf=- | ${RESULTS_DIR}/sanitize_json > "$out" diff "$out" compilable/extra-files/$name.json rm "$out"

(sanitize_json currently doesn't accept stdin)

diff can take -: https://stackoverflow.com/a/9847527/1426932

"$DMD" -X | ${RESULTS_DIR}/sanitize_json | diff - compilable/extra-files/$name.json ``

This is existing code, I think any changes to it would be a seperate PR.

ditto

wilzbach · 2018-02-04T20:48:58Z

src/dmd/mars.d

                    break;
+                case 'q':
+                    if (!p[3])
+                        goto Lnoarg;


timotheecour · 2018-02-04T20:57:22Z

src/dmd/mars.d

+
+    // Write buf to file
+    const(char)* name = global.params.jsonfilename;
+    if (name && name[0] == '-' && name[1] == 0)


use strcmp?

how about factoring this use case (probably useful for other flags; also more self-documenting)

enum stdout_alias="-; if (name && !strcmp(name, stdout_alias)) {...}

This is existing code, I think any changes to it would be a seperate PR.

timotheecour · 2018-02-04T21:16:25Z

src/dmd/json.d

+        // of modules representing their syntax.
+
+        json.arrayStart();
+        for (size_t i = 0; i < modules.dim; i++)


foreach (+ everywhere relevant)

timotheecour · 2018-02-04T21:20:34Z

src/dmd/json.d

+            auto fieldID = tryParseJsonField(fieldName);
+            if (!fieldID.hasValue)
+            {
+                json.generateError(("invalid query: unknown field name '" ~ fieldName ~ "'\0").ptr);


aren't backticks "`" used instead of "'" in messages ? (for syntax highlight)

timotheecour · 2018-02-04T21:25:59Z

src/dmd/json.d

+            auto fieldID = tryParseJsonField(fieldName);
+            if (!fieldID.hasValue)
+            {
+                json.generateError(("invalid query: unknown field name '" ~ fieldName ~ "'\0").ptr);


.ptr is not needed; D string litterals are null terminated and can decay to const(char)*
+everywhere relevant

Doesn't appear to work in this case, says it can't convert char[] to char*.

looks like it did :) (not seeing .ptr after string literals in your latest commit)
if not, i'm curious in what case it didn't work? i thought string literals would convert?

The latest doesn't using ~ to append any strings. Looks like appending strings breaks implicit conversion from string to const(char)*.

timotheecour · 2018-02-04T21:36:31Z

src/dmd/json.d

+    JsonField value;
+}
+OptionalJsonField tryParseJsonField(const(char)[] fieldName)
+{


DRY:

foreach(a; __traits(allMembers, JsonField)) if(fieldName == a) return OptionalJsonField(true, mixin(`JsonField.`~a));

done, I was thinking I needed to use phobos EnumMembers...I forgot I could do this with __traits.

timotheecour · 2018-02-04T21:59:05Z

src/dmd/mars.d

+        else
+        {
+            // Generate json file name from first obj name
+            const(char)* n = global.params.objfiles[0];


check for global.params.objfiles.length > 0 (cf to avoid thinkgs like https://github.com/dlang/dmd/pull/7839/files)

This is existing code, I think any changes to it would be a seperate PR.

ditto

timotheecour · 2018-02-04T21:59:47Z

src/dmd/mars.d

+    }
+    else
+    {
+        /* The filename generation code here should be harmonized with Module::setOutfile()


timotheecour · 2018-02-04T22:01:20Z

test/compilable/extra-files/json2.out

+        "cwd": "VALUE_REMOVED_FOR_TEST",
+        "importPaths": [
+            "compilable",
+            "..\/..\/druntime\/import",


how about making sanitize replace \/ with / ?

This is the JSON generated by std.json (not dmd). Dmd doesn't generate these weird escaped slashes. To fix this we would need to post-process the JSON after it is sanitized, basically another JSON parser that sanitizes after std.json...lol :)

lol indeed... std.json really sux; i guess a JsonPolicy to not escape / would work here (yes, unrelated to this PR)

wait, there already is an option for this! https://dlang.org/phobos/std_json.html#.JSONOptions.doNotEscapeSlashes

I'll updated the PR to use it :)

timotheecour · 2018-02-04T22:13:35Z

test/sanitize_json.d

+        foreach (ref obj; root.array)
+        {
+            auto kind = obj.object["kind"].str;
+            if (kind == "compilerInfo")


DRY + type safety:
s/"compilerInfo"/JsonField.compilerInfo.stringof/
+everywhere relevant

This is the test suite, not dmd. It doesn't have access to the JsonField type defined in the compiler.

import dmd.json : JsonField; ?

That would add a new dependency, making the compiler a dependency on the sanitizer. The sanitizer is akin to an external tool like rdmd or dub. Like the sanitizer, they are reading the JSON file using a pre-defined interface, a public interface that needs to be well-defined. These field names are apart of that interface, and a tool shouldn't need to import the compiler source code to use it.

Yeah, the testsuite shouldn't (directly) depend on what it is testing.

timotheecour · 2018-02-04T22:46:00Z

src/dmd/mars.d

+                case 'q':
+                    if (!p[3])
+                        goto Lnoarg;
+                    params.jsonQuery = p + 3 + (p[3] == '=');


check p[3] == '=' and params.jsonQuery = p + 4
We really should not allow things like -Xqfoo in new flags; this is old baggage from dmd cmd line (eg -offile instead of (-of=file), the downsides have these have been discussed several times: it's not standard, hard to read visually, and disallows future new flags (eg -Xquery=baz)

Also, -Xf- (at mentioned in toplevel msg) reads ambiguously: -o- means no output, but -Xf- as you wrote means output to stdout; with what I suggest there is no ambiguity: -Xf=- means: stdout

Yes I agree with this, I'll remove support for omitting =.

wilzbach · 2018-02-11T20:24:27Z

src/dmd/globals.d

+    compilerInfo = 0x01,
+    buildInfo    = 0x02,
+    modules      = 0x04,
+    semantics    = 0x08,


Typically binary shifts are used, s.t. it's easier for reviewers when this gets bumped.

wilzbach · 2018-02-13T05:22:58Z

src/dmd/json.d

+    string prefix = "";
+    foreach (enumName; __traits(allMembers, JsonFieldFlags))
+    {
+        if (enumName != "none")


How about:

foreach (idx, enumName; __traits(allMembers, JsonFieldFlags)) { if (idx > 0) { s ~= ", " ~ enumName; } }

Hmmm, that produces a different string ", compilerInfo, ..." instead of "compilerInfo, ..."

Ah it's too late here. I meant:

if (idx > 0) s ~= ", "; s ~= enumName;

(the main point was about not hard-coding that the first enumName is named "none")

Oh...but now you're hardcoding that the first member should be ignored...is that better than hardcoding that the value named "none" should be ignored?

Eh...I suppose I don't have a preference for either...so we'll go with your preference :)

wilzbach · 2018-02-13T05:24:20Z

src/dmd/json.d

+    auto fieldNameString = fieldName[0 .. strlen(fieldName)];
+    foreach (enumName; __traits(allMembers, JsonFieldFlags))
+    {
+        if (enumName != "none" && fieldNameString == enumName)


You could move if (enumName != "none") out of the loop.
That saves a few duplicated comparisons.

wilzbach · 2018-02-13T05:30:38Z

test/sanitize_json.d

+            {
+                sanitizeSemantics(semantics.object);
+            }
+        }


DRY idea:

static foreach (e; [tuple("compilerInfo", "sanitizeCompilerInfo") tuple("buildInfo", "sanitizeBuildInfo")])]) {{ auto infonfo = rootObject.get(e[0], JSONValue.init); if (compilerInfo.type != JSON_TYPE.NULL) mixin(e[1] ~ "(info.object);"); }}

It's okay to use newer compiler features here as DMD + DRuntime + Phobos are just freshly built from master.

wilzbach · 2018-02-13T06:24:26Z

test/sanitize_json.d

+    if (s.length == 0 || (s[0] < 'a' || s[0] > 'z'))
+        return s;
+    return (cast(char)(s[0] - ('a' - 'A'))) ~ s[1 .. $];
+}


What's wrong with https://dlang.org/phobos/std_string.html#.capitalize?

Looks like that one won't work, it converts the rest to lower-case...which is not what is wanted.

Ah that's unexpected. I thought there's a function for this in Phobos :/
Well, you could do this:

import std. unit : asCapitalized; return s.take(1).asCapitalized.chain(s.drop(1));

It would be more idiomatic, but put more stress on the CTFE (though it's a small standalone script for which this is probably not a concern);

Ugh...I can't get it to work. I added .array to the end but it ends up returning dchar[]...

Interesting it worked for me (https://run.dlang.io/is/xP19Bv),

the end but it ends up returning dchar[]

You can use .byCodeUnit to opt-out of auto-decoding, but to!string or text from std.conv are both smart enough (e.g. https://run.dlang.io/is/zv8RlX)

For pure ASCII, there's also std.ascii.toLower:

s.enumerate.map!(a => a.index == 0 ? a.value.toUpper : a.value).text;

https://run.dlang.io/is/rHnADh

But this is definitely nit-picking here ;-)

looks like std.conv.text did the trick. My std.range / std.algorithm 'fu could use work.

wilzbach

I still like it 👍

JinShil · 2018-02-14T04:47:50Z

This PR could use a statement regarding motivation (at least if you need my review). I don't have an intimate understanding of the problem this solves or the use case. Also, why do we need multiple options? Is there a problem with just generating everything?

Also, I don't see why it shouldn't be documented. What's the motivation for keeping it hidden? Are we not sure if it's a good idea?

The code is not 100% covered by tests. Can the lack of coverage be justified?

Overall, though, I don't have any issue with this PR, and I'm not objecting to it; I'm actually trying to move it forward by adding my support. But I need some understanding (answers to the questions above) before I can support it.

marler8997 · 2018-02-14T05:29:05Z

This PR could use a statement regarding motivation (at least if you need my review)

This PR is a result of a suggestion from @WalterBright . We needed a way of capturing information from the compiler without redirecting stdout/stderr. I suggested providing a way to redirect verbose output to a file but Walter suggested we add the information to the JSON file, so this PR is the result.

Also, why do we need multiple options? Is there a problem with just generating everything?

Generating everything is fine, but the output is meant for tools that can process the JSON output. So it makes sense to allow the tool to specify what it wants. This also has the advantage that the tool won't need to pay the overhead to generate everything if they don't need it. For example, dub only needs to generate compilerInfo and generating it quickly will likely be a requirement. Also it provides a mechanism to allow a tool to run the compiler with no source files, something DUB will do to query the compiler information. It also maintains backwards compatibility. Existing use cases where JSON is generated is unnafected, it generates the original format, but if a tool uses -Xi then it will generate the new format with the requested fields. And it will remain forward compatible because any new fields that might be added later will not be included since they won't be requested via -Xi.

Also, I don't see why it shouldn't be documented. What's the motivation for keeping it hidden? Are we not sure if it's a good idea?

The only reason for not documenting it is no one has presented a good use case for this feature to be used out in the wild. The only current use cases are dub and rdmd, both of which are internal tools. If it deemed to be useful outside of internal use, then documenting it makes sense. I'm basically taking the conservative approach where I'm not including any changes that can always be decided later.

The code is not 100% covered by tests. Can the lack of coverage be justified?

I haven't checked the code coverage. Depends on which code paths aren't taken, I'll see if I can take a look.

JinShil · 2018-02-14T07:30:02Z

Thanks @marler8997

Who are the primary consumers of this JSON output and for what purpose? You mentioned dub and rdmd, so what do they need with this information?

What about feature overlap? Isn't this information something 3rd party tools can obtain for themselves using DMD-as-a-library?

marler8997 · 2018-02-14T07:37:35Z

rdmd will use it to output all the files used during a compilation. It uses this for dependency analysis. It will also use compilerInfo to know what features a particular compiler binary supports. Same with dub.

Dmd as a library really isn't applicable for these use cases. The JSON output is used to get information about a particular compiler binary or a particular build, whereas dmd as a library enables an application to have access to compiler functionality.

JinShil · 2018-02-14T08:52:41Z

src/dmd/mars.d

+        {
+            if (global.params.objfiles.dim == 0)
+            {
+                error(Loc.initial, "cannot determine JSON filename, use -Xf=<file> or provide a source file");


Tick marks: e.g. `-Xf=`

JinShil · 2018-02-14T08:55:33Z

test/fail_compilation/jsonBadField1.d

+REQUIRED_ARGS: -Xi=UNKNOWN_FIELD_NAME
+TEST_OUTPUT:
+---
+Error: unknown JSON field `-Xi=UNKNOWN_FIELD_NAME`, expected one of compilerInfo, buildInfo, modules, semantics


Please modify the code to surround each expected option in tick marks, e.g. `compilerInfo`, `buildInfo`, `modules`, `semantics`

done, thanks for the suggestion

Fix issue 18367

wilzbach · 2018-02-14T16:19:24Z

This also has the advantage that the tool won't need to pay the overhead to generate everything if they don't need it. For example, dub only needs to generate compilerInfo and generating it quickly will likely be a requirement.

Yes, at the moment Dub creates and compiles a probing file and invocates DMD on it to parse the pragma(msg) output every time before it builds something (see #7521).

For example,

There are many other potential use cases - run.dlang.io is another or basically any build tool that needs more information about the currently installed D compiler.

Also, I don't see why it shouldn't be documented. What's the motivation for keeping it hidden? Are we not sure if it's a good idea?

Yep this was done, s.t. we have one or two releases to experiment with until we set it into stone.

JinShil · 2018-02-14T16:44:49Z

Thank you @marler8997 and @wilzbach. I see where this is going now. I'm just waiting on coverage.

marler8997 requested review from MartinNowak, dnadlinger, ibuclaw, mathias-lang-sociomantic and rainers as code owners February 4, 2018 16:55

marler8997 requested a review from WalterBright as a code owner February 4, 2018 16:55

marler8997 mentioned this pull request Feb 4, 2018

Implement -probe for showing supported features #7521

Closed

marler8997 force-pushed the jsonQuery branch from 82bc929 to b618aeb Compare February 4, 2018 17:05

marler8997 force-pushed the jsonQuery branch from b618aeb to 9bf4017 Compare February 4, 2018 17:23

wilzbach approved these changes Feb 4, 2018

View reviewed changes

src/dmd/json.d Outdated

}

/**

Returns true if `c` is a valid character for the start of a name..

Copy link

Contributor

wilzbach Feb 4, 2018

Choose a reason for hiding this comment

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

double .

marler8997 force-pushed the jsonQuery branch from 9bf4017 to 0f486ac Compare February 4, 2018 17:42

wilzbach mentioned this pull request Feb 4, 2018

Issue 18368 - -X should print all static information on stdout if no files are given #7840

Closed

timotheecour reviewed Feb 4, 2018

View reviewed changes

wilzbach reviewed Feb 4, 2018

View reviewed changes

timotheecour reviewed Feb 4, 2018

View reviewed changes

src/dmd/mars.d

}

else

{

/* The filename generation code here should be harmonized with Module::setOutfile()

Copy link

Contributor

timotheecour Feb 4, 2018

Choose a reason for hiding this comment

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

yup!

timotheecour reviewed Feb 4, 2018

View reviewed changes

This was referenced Feb 4, 2018

[WIP] Investigate GraphQL for JSON introspection/filtering #7830

Closed

Fix Issue 18367 - dmd should not segfault on -X with libraries, but no source files #7839

Closed

wilzbach mentioned this pull request Feb 11, 2018

Print DFLAGS on -v to ease debugging #7865

Merged

wilzbach reviewed Feb 13, 2018

View reviewed changes

marler8997 force-pushed the jsonQuery branch 6 times, most recently from 6fa64f8 to aec1bb7 Compare February 13, 2018 06:19

wilzbach reviewed Feb 13, 2018

View reviewed changes

wilzbach approved these changes Feb 13, 2018

View reviewed changes

marler8997 force-pushed the jsonQuery branch from aec1bb7 to 2b00495 Compare February 13, 2018 07:03

JinShil suggested changes Feb 14, 2018

View reviewed changes

JinShil removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 14, 2018

Added -Xi to include new fields in JSON output

7c09995

Fix issue 18367

marler8997 force-pushed the jsonQuery branch from 2b00495 to 939284c Compare February 14, 2018 16:29

marler8997 force-pushed the jsonQuery branch 6 times, most recently from 161662f to 7c09995 Compare February 14, 2018 21:27

JinShil approved these changes Feb 15, 2018

View reviewed changes

wilzbach merged commit 9d7403f into dlang:master Feb 15, 2018

wilzbach mentioned this pull request Mar 13, 2018

Use supportedFeatures object for feature detection #8022

Merged

Uh oh!

Conversation

marler8997 commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

marler8997 commented Feb 4, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour Feb 4, 2018 • 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!

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!

marler8997 Feb 5, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour Feb 4, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

timotheecour Feb 4, 2018 • 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!

timotheecour Feb 4, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

marler8997 Feb 5, 2018 • 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!

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!

timotheecour Feb 4, 2018 • 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!

marler8997 commented Feb 4, 2018 •

edited

Loading

dlang-bot commented Feb 4, 2018 •

edited

Loading

timotheecour Feb 4, 2018 •

edited

Loading

marler8997 Feb 5, 2018 •

edited

Loading

timotheecour Feb 4, 2018 •

edited

Loading

timotheecour Feb 4, 2018 •

edited

Loading

timotheecour Feb 4, 2018 •

edited

Loading

marler8997 Feb 5, 2018 •

edited

Loading

timotheecour Feb 4, 2018 •

edited

Loading

timotheecour Feb 5, 2018 •

edited

Loading

timotheecour Feb 4, 2018 •

edited

Loading

timotheecour Feb 4, 2018 •

edited

Loading

wilzbach Feb 11, 2018 •

edited

Loading