[AST][JSON] Avoid crash when dumping NULL Type as JSON

[sidorovd]

LGTM. I'm not an expert in JSON, but may be it makes sense to move the change a line earlier before creation of pointer representation?

[lebedev.ri]

Test coverage missing.

[aaron.ballman]

LGTM, but missing a test case.

In D66850#1648557, @sidorovd wrote:

LGTM. I'm not an expert in JSON, but may be it makes sense to move the change a line earlier before creation of pointer representation?

I would prefer it remains where it is -- having the 0x0 in the output for a null pointer is a good thing because it conveys more information than a totally empty Type object. We're accidentally inconsistent about this currently (Decl prints 0x0 but Stmt gives an empty object).

[piscisaureus]

In D66850#1648776, @aaron.ballman wrote:

LGTM, but missing a test case.

In D66850#1648557, @sidorovd wrote:

LGTM. I'm not an expert in JSON, but may be it makes sense to move the change a line earlier before creation of pointer representation?

I would prefer it remains where it is -- having the 0x0 in the output for a null pointer is a good thing because it conveys more information than a totally empty Type object. We're accidentally inconsistent about this currently (Decl prints 0x0 but Stmt gives an empty object).

I don't disagree, but I would argue that "0x0" is a rather poor choice, since now the consumer has to treat the "id" field as an opaque value, except when it's "0x0". A better choice would be to use JavaScript null to represent null pointers.
That said, I've simply followed the pattern established in visit(Decl*) here; switching to null would warrant a separate patch IMO.

[piscisaureus]

Add test

[aaron.ballman]

In D66850#1649708, @piscisaureus wrote:

In D66850#1648776, @aaron.ballman wrote:

LGTM, but missing a test case.

In D66850#1648557, @sidorovd wrote:

LGTM. I'm not an expert in JSON, but may be it makes sense to move the change a line earlier before creation of pointer representation?

I would prefer it remains where it is -- having the 0x0 in the output for a null pointer is a good thing because it conveys more information than a totally empty Type object. We're accidentally inconsistent about this currently (Decl prints 0x0 but Stmt gives an empty object).

I don't disagree, but I would argue that "0x0" is a rather poor choice, since now the consumer has to treat the "id" field as an opaque value, except when it's "0x0". A better choice would be to use JavaScript null to represent null pointers.

Normally, I'd say yes, but in this case, we have to represent the pointer as a string because the number type in JSON is a signed value. Rather than "you'll either get a string or null", it seemed a bit more friendly to say "you'll always get a string" and allow the consumer to decide how to handle degenerate values. That said, I don't feel super strongly about this.

In D66850#1649804, @piscisaureus wrote:

Add test

Thank you!

LGTM, do you need someone to commit on your behalf?

[piscisaureus]

In D66850#1649865, @aaron.ballman wrote:

In D66850#1649708, @piscisaureus wrote:

In D66850#1648776, @aaron.ballman wrote:

LGTM, but missing a test case.

In D66850#1648557, @sidorovd wrote:

LGTM. I'm not an expert in JSON, but may be it makes sense to move the change a line earlier before creation of pointer representation?

I would prefer it remains where it is -- having the 0x0 in the output for a null pointer is a good thing because it conveys more information than a totally empty Type object. We're accidentally inconsistent about this currently (Decl prints 0x0 but Stmt gives an empty object).

I don't disagree, but I would argue that "0x0" is a rather poor choice, since now the consumer has to treat the "id" field as an opaque value, except when it's "0x0". A better choice would be to use JavaScript null to represent null pointers.

Normally, I'd say yes, but in this case, we have to represent the pointer as a string because the number type in JSON is a signed value. Rather than "you'll either get a string or null", it seemed a bit more friendly to say "you'll always get a string" and allow the consumer to decide how to handle degenerate values. That said, I don't feel super strongly about this.

I agree using numbers isn't feasible. Not because javascript numbers are signed, but because they're 64-bit floating point values, which means they can't faithfully store 64-bit int values. The latest versions of ECMAScript support arbitrary-precision integers a.k.a. BigInt (written as 0xABCABCABCABCABCABCn) which can represent 64-bit values, but support isn't currently widespread.

The reason I suggest to use null (which is not a number nor a string, but a type of its own) is that users need to treat the id as an opaque value (essentially they're only usable to compare nodes and use it as a lookup key in a map), with the exception of "0x0" which has a specific meaning. As a frequent JavaScript/TypeScript user, I would say that using different types is actually more ergonomic than using a string and having to know about certain strings that indicate a degenerate case.

In D66850#1649804, @piscisaureus wrote:

Add test

Thank you!

LGTM, do you need someone to commit on your behalf?

Yes, please; I have no idea how to commit things to the LLVM tree.
BTW: I haven't ran the full test suite, I suppose some CI system will do that before this gets landed?

[piscisaureus]

Add missing CHECK_NEXT line (test passes now).

[piscisaureus]

@aaron.ballman
I was able to run the tests, I think this is good to go.
Can you help me get it landed?

[aaron.ballman]

In D66850#1651519, @piscisaureus wrote:

@aaron.ballman
I was able to run the tests, I think this is good to go.
Can you help me get it landed?

Happy to do so! I landed it in r370401. Thank you for the patch!

clang/lib/AST/JSONNodeDumper.cpp

if (const auto *E = dyn_cast<Expr>(S)) {
     }
     JOS.attribute("valueCategory", Category);
   }
   InnerStmtVisitor::Visit(S);
 }
 
 void JSONNodeDumper::Visit(const Type *T) {
   JOS.attribute("id", createPointerRepresentation(T));
+
+  if (!T)
+    return;
+
   JOS.attribute("kind", (llvm::Twine(T->getTypeClassName()) + "Type").str());
   JOS.attribute("type", createQualType(QualType(T, 0), /*Desugar*/ false));
   attributeOnlyIfTrue("isDependent", T->isDependentType());
   attributeOnlyIfTrue("isInstantiationDependent",
                       T->isInstantiationDependentType());
   attributeOnlyIfTrue("isVariablyModified", T->isVariablyModifiedType());
   attributeOnlyIfTrue("containsUnexpandedPack",
                       T->containsUnexpandedParameterPack());

clang/test/AST/ast-dump-types-json.cpp

 #define CDECL __attribute__((cdecl))
 typedef void (CDECL *TestMacroQualifiedType)();
 
 typedef void (T::* TestMemberFunctionPointerType)();
 typedef int T::*TestMemberDataPointerType;
 
 typedef int TestQualTypePrinting(const char* c);
 
+typedef int TestUsingShadowDeclType;
+namespace TestNamespaceWithUsingShadowType {
+using ::TestUsingShadowDeclType;
+}
+
 // NOTE: CHECK lines have been autogenerated by gen_ast_dump_json_test.py
 
 
 // CHECK:  "kind": "TypedefDecl",
 // CHECK-NEXT:  "loc": {
 // CHECK-NEXT:   "file": "{{.*}}",
 // CHECK-NEXT:   "line": 12,
 // CHECK-NEXT:   "col": 18,

 // CHECK-NEXT:        ]
 // CHECK-NEXT:       }
 // CHECK-NEXT:      ]
 // CHECK-NEXT:     }
 // CHECK-NEXT:    ]
 // CHECK-NEXT:   }
 // CHECK-NEXT:  ]
 // CHECK-NEXT: }
+
+// CHECK:  "kind": "NamespaceDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "file": "{{.*}}",
+// CHECK-NEXT:   "line": 24,
+// CHECK-NEXT:   "col": 11,
+// CHECK-NEXT:   "tokLen": 32
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:    "col": 1,
+// CHECK-NEXT:    "tokLen": 9
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:    "line": 26,
+// CHECK-NEXT:    "col": 1,
+// CHECK-NEXT:    "tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "TestNamespaceWithUsingShadowType",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:    "id": "0x{{.*}}",
+// CHECK-NEXT:    "kind": "UsingDecl",
+// CHECK-NEXT:    "loc": {
+// CHECK-NEXT:     "line": 25,
+// CHECK-NEXT:     "col": 9,
+// CHECK-NEXT:     "tokLen": 23
+// CHECK-NEXT:    },
+// CHECK-NEXT:    "range": {
+// CHECK-NEXT:     "begin": {
+// CHECK-NEXT:      "col": 1,
+// CHECK-NEXT:      "tokLen": 5
+// CHECK-NEXT:     },
+// CHECK-NEXT:     "end": {
+// CHECK-NEXT:      "col": 9,
+// CHECK-NEXT:      "tokLen": 23
+// CHECK-NEXT:     }
+// CHECK-NEXT:    },
+// CHECK-NEXT:    "name": "::TestUsingShadowDeclType"
+// CHECK-NEXT:   },
+// CHECK-NEXT:   {
+// CHECK-NEXT:    "id": "0x{{.*}}",
+// CHECK-NEXT:    "kind": "UsingShadowDecl",
+// CHECK-NEXT:    "loc": {
+// CHECK-NEXT:     "col": 9,
+// CHECK-NEXT:     "tokLen": 23
+// CHECK-NEXT:    },
+// CHECK-NEXT:    "range": {
+// CHECK-NEXT:     "begin": {
+// CHECK-NEXT:      "col": 9,
+// CHECK-NEXT:      "tokLen": 23
+// CHECK-NEXT:     },
+// CHECK-NEXT:     "end": {
+// CHECK-NEXT:      "col": 9,
+// CHECK-NEXT:      "tokLen": 23
+// CHECK-NEXT:     }
+// CHECK-NEXT:    },
+// CHECK-NEXT:    "isImplicit": true,
+// CHECK-NEXT:    "target": {
+// CHECK-NEXT:     "id": "0x{{.*}}",
+// CHECK-NEXT:     "kind": "TypedefDecl",
+// CHECK-NEXT:     "name": "TestUsingShadowDeclType"
+// CHECK-NEXT:    },
+// CHECK-NEXT:    "inner": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:      "id": "0x0"
+// CHECK-NEXT:     }
+// CHECK-NEXT:    ]
+// CHECK-NEXT:   }
+// CHECK-NEXT:  ]
+// CHECK-NEXT: }

clang/test/AST/gen_ast_dump_json_test.py

for k, v in dict_var.items():
         if isinstance(v, OrderedDict):
             normalize(v)
         elif isinstance(v, list):
             for e in v:
                 if isinstance(e, OrderedDict):
                     normalize(e)
         elif type(v) is unicode:
             st = v.encode('utf-8')
-            if re.match(r"0x[0-9A-Fa-f]+", v):
+            if v != "0x0" and re.match(r"0x[0-9A-Fa-f]+", v):
                 dict_var[k] = u'0x{{.*}}'
             elif os.path.isfile(v):
                 dict_var[k] = u'{{.*}}'
             else:
                 splits = (v.split(u' '))
                 out_splits = []
                 for split in splits:
                     inner_splits = split.rsplit(u':',2)