Add more stuff to JSON file #7757

marler8997 · 2018-01-20T16:20:08Z

This PR is based on recommendation from @WalterBright (#7746 (comment)). In order for rdmd to support single-invocation, we need a way to get the information it currently gets from "verbose output", but without losing error messages and other stdout messages. Walter recommended rdmd switch to using the JSON file that dmd generates. Currently the JSON file is missing most of the information that rdmd requires, so this PR is adding that information to it.

Most of the information needed by rdmd is "semantic data" whereas the JSON file seems to contain "syntax data". Because of this, I've added the "semantic data" to the end of the JSON file in it's own object called semantics. Here's a list of the new additions and the current status:

non-root imported modules (ADDED)
file content imports (i.e. import("somefile")) (ADDED)

Along with semantic data, there is some build information that rdmd uses. I've put this in the beginning of the JSON file in an object named "buildInfo". It contains the following:

target library file if -lib is used (ADDED)
the compiler config file (ADDED)
the compiler binary file itself (ADDED)

dlang-bot · 2018-01-20T16:20:08Z

Thanks for your pull request, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

marler8997 · 2018-01-20T17:07:38Z

Hmmm, this is going to make testing more difficult. Now that the JSON file contains semantic data, it's contents are different for each platform, and also changes depending on the order that files were passed to the compiler.

timotheecour · 2018-01-20T18:27:27Z

1 quick sanity check: seeing whether on a large project, adding -Xf=file slows down compilation (or consumes more memory ?) than without. Because each function declaeation (+other things) gets serialized. If so, we'd need a syntax like -X=imports,versions,libs to control what gets exported. in any case, its a less gacky version than -v

…

On Jan 20, 2018 9:07 AM, "Jonathan Marler" ***@***.***> wrote: Hmmm, this is going to make testing more difficult. Now that the JSON file contains semantic data, it's contents are different for each platform, and also changes depending on the order that files were passed to the compiler. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7757 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACF9YOAI8HLQmmMtatlOxF82YoAQpIdkks5tMh1bgaJpZM4RlfaI> .

marler8997 · 2018-01-20T18:39:24Z

Based on the code, I don't anticipate json code generation having a big performance impact. However, if it does then filtering can always be added later as an after thought. Maybe:

-X (enable json code generation)
-Xf=<file> (set file to output json to)
-Xo=-functions  (An example interface to filter out function from json code generation)

CyberShadow · 2018-01-20T20:45:24Z

1 quick sanity check: seeing whether on a large project, adding -Xf=file slows down compilation (or consumes more memory ?) than without.

I checked on ae (dmd -o- -deps -X all.d where all.d is generated by makejson.sh), and saw no measurable impact on execution time and a negligible (200 KB) increase in memory usage, so using the JSON output looks pretty promising!

CyberShadow · 2018-01-20T22:11:37Z

-Xo=-functions (An example interface to filter out function from json code generation)

BTW, if we ever do need to add that, it should probably be a list of things to include rather than a list of things to exclude. Tools that would make use of this option will know exactly what they want, so any information that's added later will not be useful to them.

marler8997 · 2018-01-20T22:13:04Z

yeah good point

marler8997 · 2018-01-21T21:11:39Z

@WalterBright We may want to try to give this PR some attention early on because if this PR along with the changes to rdmd don't get in before the next release, then we will have an extra use case to handle, namely, the case where you have a compiler that supports -i but doesn't generate the JSON output that is needed. I'd like to avoid that if possible.

timotheecour · 2018-01-22T06:14:29Z

src/dmd/json.d

+        {
+            objectStart();
+            if(m.md)
+                property("name", m.md.toChars());


how about:
property("name", m.md ? m.md.toChars() : "");
to make each entry have each field? (and update https://github.com/dlang/tools/pull/292/files#diff-e6f94050037e7c9a64684829764b1f0bR832 accordingly)

I was following the convention established here (https://github.com/dlang/dmd/pull/7757/files/a90c983d6e436e46dde3b1ee07fdd280ea1396e7#diff-1d73c979f233c43e47fb88fac32c21b3L475)

I don't really see much difference whether or not there is an empty name or no name at all, but following the existing convention seems the better choice.

timotheecour · 2018-01-22T06:20:54Z

src/dmd/json.d

+            objectStart();
+            if(m.md)
+                property("name", m.md.toChars());
+            property("file", m.srcfile.toChars());


how about absolutePath ( m.srcfile.toChars() ) so that if rdmd is called from a different directory, it'll still work?

hmmmm, interesting thought. I'd have to think about that one. This is mimicking the current functionality from the verbose output, but maybe it can be improved. I'll come back to this.

NOTE: "contentImports" appears to use absolutePath (i tried with -J. and import("foo.txt") ), so I would make sense to be consistent. I suggest using absolutePath for file as well (and wherever else it makes sense)

I believe that would be changing behavior though...we could end up with a regression if we change the behavior.

well, that info wasn't in json output before... so it's one of those gray area things.
one option is to allow configuring json output (as I suggested somewhere else), which is OK if we have good (easily extensible) way to customize, eg:

-Xf=file -Xopt=abspaths,imports,-functions
each option has a corresponding - flag to negate it, just like you did for -i=core,-std

Now that I think about it, there might be a bug in the current design. If you build with rdmd, and then you change directories and build again, then all the relative module filenames will be incorrect. It might just force you to do a rebuild, but that's no good. We could fix this using absolute paths, or we could save off the original CWD that rdmd was using when it first performed the build.

There should be a test for that in the rdmd test suite, FWIW.

there may even be corner cases where it picks up wrong file eg:

ls
main.d
fun.d
temp/fun.d // slightly different version main.d

rdmd main.d
cd temp
rdmd ../main.d
=> can it get confused and get the wrong fun.d?

Here's a good compromise:

add a CWD field, and when rdmd reads a path (any path) call buildPath(json.CWD, json.path) (does the right thing w absolute paths)

and allow for:
rdmd main.d
cd temp
rdmd ../main.d => should reuse cache and not rebuild

but we should definitely make behavior consistent between contentImports and modules.file:
import("fun.txt") should not get auto-transformed in an absolute path either.

I'm thinking that the JSON file saves the CWD in the new buildInfo object.

timotheecour · 2018-01-22T07:57:45Z

src/dmd/json.d

+                arrayStart();
+                foreach (file; m.contentImportedFiles)
+                {
+                    value(file);


BUG! missing separaters in generated JSON
"contentImports" : [
"/tmp/myfile.txt""/tmp/myfile.txt"
]

wilzbach · 2018-01-22T17:34:05Z

test/compilable/extra-files/gen_json_out.d

+ {
+  "kind" : "buildInfo",
+  "binary" : "../generated/linux/release/64/dmd",
+  "version" : "v2.078.1-beta.1-494-g479722a-dirty",


You probably want to have frontendVersion here too.

Also you can't check for neither the version nor binary in the testsuite - they are supposed to change ;-)

they are "grepped" out along with some other types of lines (see json-postscript.sh). I thought keeping an example in the actual expected results didn't hurt though.

wilzbach

Do we really need to generate the JSON file?
It makes future updates of the JSON file more difficult as the line numbers on the diff won't reflect the ones in the generated script. Also it's easy to screw up the JSON generation. How about moving the things that are platform-specific into a different test?

marler8997 · 2018-01-30T02:25:13Z

@andralex has determined that adding policies to std.json is not a desirable enhancement (dlang/phobos#6059 (comment)). Given that now we can't use std.json to sanitize the file, using the provided JSON parser seems like a solid option to me.

CyberShadow · 2018-01-30T02:28:12Z

Given that now we can't use std.json to sanitize the file,

Sorry, I don't think we finished the previous conversation. Why not use std.json, and rely on its current behavior of sorting key names lexicographically? Won't that allow achieving the same goal without adding a JSON parser to the DMD repository?

marler8997 · 2018-01-30T02:30:42Z

That would work "functionally". The only drawback is that the two files won't be in the same order that was generated from DMD, which would look odd. The order of the fields as generated by DMD makes sense, i.e. name first, followed by other identifying properties, followed by more verbose fields such as large objects. Would be nice to keep it in the same order. I refuse to accept that sanitizing a JSON file while preserving the original object field order is an insurmountable task in D :)

CyberShadow · 2018-01-30T02:34:46Z

The only drawback is that the two files won't be in the same order that was generated from DMD, which would look odd.

IMHO that's fine, considering the circumstances, but it's not up to me to decide.

I refuse to accept that sanitizing a JSON file while preserving the original object field order is an insurmountable task in D :)

Certainly not insurmountable, but it may be worth keeping our feet on the ground in this case. It's just a test case file that hopefully won't be edited / looked at often at all, so maybe just go for the simplest solution.

marler8997 · 2018-01-30T02:38:04Z

Certainly not insurmountable, but it may be worth keeping our feet on the ground in this case. It's just a test case file that hopefully won't be edited / looked at often at all, so maybe just go for the simplest solution.

Yes I see your point. I'll concede this and go with your solution. Be prepared for a large json.out diff :)

marler8997 · 2018-01-30T03:34:43Z

Ok I caved, the JSON sanitizer uses std.json so all the object fields are sorted alphabetically.

marler8997 · 2018-02-02T02:15:56Z

Ping. Trying to get this PR finalized and integrated so we have some time before the 2.079 release to modify rdmd to use it (dlang/tools#292).

JinShil · 2018-02-02T02:58:58Z

Restarted Jenkins

wilzbach

Looks good, but please add a short changelog entry.

wilzbach · 2018-02-02T03:22:58Z

test/sanitize_json.d

+        *fileNode = JSONValue(normalizeFile(fileNode.str));
+        newModules.put(JSONValue(semanticModule));
+    }
+    *modulesArrayPtr = newModules.data;


Holy moly, is std.json ugly. We really need to put a warning in this module to prevent people from using it.

wilzbach · 2018-02-02T03:25:53Z

test/compilable/extra-files/json.out

@@ -1,930 +1,976 @@
 [


FYI for the future: typically automatically run commands are separated in different commits, s.t. reviewers don't have to look at them too closely. In this case, GH hides the file anyways.

wilzbach · 2018-02-02T15:07:34Z

test/sanitize_json.d

+            else if (name == "offset")
+                removeNumber(&value.object[name]);
+            else
+                sanitizeSyntaxNode(value.object[name]);


Not a fan of this (I know that's how the status quo is).
Ideally this would be configurable via a flag, s.t. special platform-specific, short tests which test that the deco is correctly set in the json output can be added, but that's definitely not something which shouldn't block this PR.

Ok, I added a --keep-deco flag that can be used by tests in the future.

marler8997 · 2018-02-02T16:41:51Z

Added the changelog

wilzbach

Thanks a lot for putting all the hard work into this!
I'm going to merge this now to unblock your work on rdmd - could you do a quick follow up to cover the uncovered output of the imported files? Thanks!

wilzbach · 2018-02-02T18:26:33Z

src/dmd/json.d

+                foreach (file; m.contentImportedFiles)
+                {
+                    item(file);
+                }


This isn't covered.

timotheecour · 2018-02-02T21:20:00Z

test/sanitize_json.d

+    }
+    else
+    {
+        writeln("Error: too many command line arguments");


marler8997 mentioned this pull request Jan 20, 2018

Allow verbose output to be directed to a file #7746

Closed

marler8997 force-pushed the extendJson branch from b54b716 to d017a30 Compare January 20, 2018 16:41

marler8997 requested review from ibuclaw and dnadlinger as code owners January 20, 2018 16:41

marler8997 force-pushed the extendJson branch from d017a30 to 6218580 Compare January 20, 2018 17:06

marler8997 force-pushed the extendJson branch from 6218580 to ca63c46 Compare January 20, 2018 17:08

marler8997 force-pushed the extendJson branch 6 times, most recently from 479722a to 9a297d6 Compare January 20, 2018 20:20

marler8997 force-pushed the extendJson branch from 9a297d6 to a90c983 Compare January 20, 2018 22:22

marler8997 mentioned this pull request Jan 21, 2018

Add support for the -i interface and maintain backwards compatibility. dlang/tools#292

Closed

timotheecour reviewed Jan 22, 2018

View reviewed changes

marler8997 force-pushed the extendJson branch 2 times, most recently from 7bb732c to 8be0d2d Compare January 22, 2018 08:19

timotheecour mentioned this pull request Jan 22, 2018

Implement -probe for showing supported features #7521

Closed

wilzbach reviewed Jan 22, 2018

View reviewed changes

marler8997 force-pushed the extendJson branch 7 times, most recently from 4f8dfc7 to 3e18689 Compare January 27, 2018 22:17

marler8997 force-pushed the extendJson branch from 3e18689 to 2bc4cf9 Compare January 30, 2018 03:32

marler8997 force-pushed the extendJson branch from 2bc4cf9 to 41aaf97 Compare January 30, 2018 03:56

Extend JSON format to include relevant information for tools like rdmd.

24d9d18

wilzbach approved these changes Feb 2, 2018

View reviewed changes

wilzbach added the Needs Changelog A changelog entry needs to be added to /changelog label Feb 2, 2018

marler8997 force-pushed the extendJson branch 2 times, most recently from 1471901 to dc7fb7a Compare February 2, 2018 16:40

marler8997 force-pushed the extendJson branch from dc7fb7a to 24d9d18 Compare February 2, 2018 16:42

marler8997 mentioned this pull request Feb 2, 2018

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

Closed

wilzbach removed the Needs Changelog A changelog entry needs to be added to /changelog label Feb 2, 2018

wilzbach approved these changes Feb 2, 2018

View reviewed changes

src/dmd/json.d

foreach (file; m.contentImportedFiles)

{

item(file);

}

Copy link

Member

wilzbach Feb 2, 2018

Choose a reason for hiding this comment

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

This isn't covered.

wilzbach merged commit cca2a1e into dlang:master Feb 2, 2018

timotheecour reviewed Feb 2, 2018

View reviewed changes

test/sanitize_json.d

}

else

{

writeln("Error: too many command line arguments");

Copy link

Contributor

timotheecour Feb 2, 2018

Choose a reason for hiding this comment

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

stderr?

marler8997 deleted the extendJson branch February 6, 2018 18:03

Add more stuff to JSON file #7757

Add more stuff to JSON file #7757

Conversation

marler8997 commented Jan 20, 2018 • edited Loading

dlang-bot commented Jan 20, 2018

Bugzilla references

marler8997 commented Jan 20, 2018

timotheecour commented Jan 20, 2018 via email

marler8997 commented Jan 20, 2018

CyberShadow commented Jan 20, 2018

CyberShadow commented Jan 20, 2018

marler8997 commented Jan 20, 2018

marler8997 commented Jan 21, 2018

timotheecour Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

marler8997 commented Jan 30, 2018 • edited Loading

CyberShadow commented Jan 30, 2018

marler8997 commented Jan 30, 2018

CyberShadow commented Jan 30, 2018

marler8997 commented Jan 30, 2018 • edited Loading

marler8997 commented Jan 30, 2018

marler8997 commented Feb 2, 2018

JinShil commented Feb 2, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 commented Feb 2, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 commented Jan 20, 2018 •

edited

Loading

timotheecour Jan 22, 2018 •

edited

Loading

timotheecour Jan 22, 2018 •

edited

Loading

wilzbach Jan 22, 2018 •

edited

Loading

marler8997 commented Jan 30, 2018 •

edited

Loading

marler8997 commented Jan 30, 2018 •

edited

Loading