Adding a validation pass to check types, field IDs and Identifiers #70

kevinbgreene · 2017-09-20T19:43:30Z

validate that types are being assigned properly
validate all identifiers can be resolved
validate no duplicate field IDs
validate correct enum usage

As part of this I also fix initializing i64 fields to numbers, now Int64.

* validate that types are being assigned properly * validate all identifiers can be resolved * validate no duplicate field IDs * validate correct enum usage

phated

Looks like a 0-line file debugger.ts got committed. Also made a few inline comments about some things I saw. Haven't run the test suite locally - I'd love to get this on CI.

phated · 2017-09-20T21:27:54Z

src/app/render/values.ts

    case SyntaxType.DoubleConstant:
-      return createLiteral(node.value)
+      return ts.createLiteral(node.value)

    case SyntaxType.ConstList:


The parser should differentiate between ConstList and ConstSet for the future.

The IDL only defines ConstList. It's more context awareness than I would like to have the parser add a ConstSet. It should be rendering a Set here though, don't know why I missed that/how I'm not testing for it. Will fix.

phated · 2017-09-20T21:32:20Z

src/app/render/values.ts

    ])
  })

-  return createNew(
+  return ts.createNew(


Random thought: does Scrooge make this improvement to Maps? If not, this probably needs to generate a plain object until Maps and Sets are used consistently and all normalization applied.

This new Map change was my improvement/fix and required more changed within the codegen output.

Yeah, scrooge uses Map instead of object literals.

phated · 2017-09-20T21:36:08Z

src/app/resolver.ts

@@ -291,12 +320,15 @@ function createResolver(thrift: ThriftDocument, includes: IIncludeMap): IResolve
    return false
  }

+  /**
+   *
+   * @param name


Mistakenly inserted?

phated · 2017-09-20T21:49:30Z

src/app/validator/index.ts

+          type: SyntaxType.ServiceDefinition,
+          name: statement.name,
+          functions: validateFunctions(statement.functions),
+          extends: statement.extends,


I think extends should be validated also. You can extend an included service.

Good catch.

phated · 2017-09-20T21:51:03Z

src/app/validator/index.ts

+    if (fieldID === null) {
+      return {
+        type: SyntaxType.FieldID,
+        value: (--generatedFieldID),


generatedFieldID should be pushed onto usedFieldIDs after it's used.

I should be checking that the user supplied id is not negative right. Negative IDs are reserved for generation if I remember correctly?

phated · 2017-09-20T22:00:54Z

src/app/validator/index.ts

+  }
+
+  function validateFields(fields: Array<FieldDefinition>): Array<FieldDefinition> {
+    generatedFieldID = 0


This seems fine as long as Thrift continues to not support nested struct definition.

Yeah, the scope of generatedFieldID and usedFieldIDs is probably too broad. I'll fix that.

phated · 2017-09-20T22:01:22Z

src/app/validator/index.ts

+      return {
+        type: SyntaxType.FunctionDefinition,
+        name: func.name,
+        oneway: func.oneway,


Is this being validated with void type?

The parser is checking for oneway == void now. I'm thinking about getting rid of that check in the parser and moving all of that sort of validation here. Thoughts?

* Should check extends identifier to ensure it is referencing a service * Should check that user created field IDs are non-negative * Get rid of the nasty scoping around generated field IDs * Should be rendering Sets where sets have default values (was using array)

ian-harshman-ck · 2017-09-20T23:51:48Z

src/app/render/struct/read.ts

@@ -347,6 +347,9 @@ export function readValueForIdentifier(
    case SyntaxType.ConstDefinition:
      throw new TypeError(`Identifier ${id.definition.name.value} is a value being used as a type`)

+    case SyntaxType.ServiceDefinition:
+      throw new TypeError(`Service ${id.definition.name.value} is begin used as a type`)


begin -> beign

ian-harshman-ck · 2017-09-20T23:51:58Z

src/app/render/struct/write.ts

@@ -146,6 +146,9 @@ function writeValueForIdentifier(
    case SyntaxType.ConstDefinition:
      throw new TypeError(`Identifier ${id.definition.name.value} is a value being used as a type`)

+    case SyntaxType.ServiceDefinition:
+      throw new TypeError(`Service ${id.definition.name.value} is begin used as a type`)


begin -> being

ian-harshman-ck · 2017-09-20T23:52:08Z

src/app/render/types.ts

@@ -181,6 +181,9 @@ function thriftAccessForIdentifier(id: IIdentifierType, identifiers: IIdentifier
    case SyntaxType.ConstDefinition:
      throw new TypeError(`Identifier ${id.definition.name.value} is a value being used as a type`)

+    case SyntaxType.ServiceDefinition:
+      throw new TypeError(`Service ${id.definition.name.value} is begin used as a type`)


begin -> being

Adding a validation pass

4a68ee2

* validate that types are being assigned properly * validate all identifiers can be resolved * validate no duplicate field IDs * validate correct enum usage

kevinbgreene requested review from phated, nnance and ian-harshman-ck September 20, 2017 19:43

This was referenced Sep 20, 2017

Add a type-checking pass to the parser creditkarma/thrift-parser#3

Closed

Need to validate that field IDs are unique creditkarma/thrift-parser#28

Closed

Adding validation tests for nested containers

b0e1513

phated suggested changes Sep 20, 2017

View reviewed changes

kevin-greene-ck added 2 commits September 20, 2017 16:08

Reverting some unintended changes to test application

41994c4

ian-harshman-ck approved these changes Sep 20, 2017

View reviewed changes

kevin-greene-ck added 6 commits September 20, 2017 18:59

Fixing typos

4fb1733

Removing empty debugger file

9509a70

Adding validation for oneway functions to have void return type

ca2981e

Adding tests for generation of missing field IDs

fb52a94

Fixing unintended change

ac333f7

Consistenly use single quotes in validator test file

cf600a5

kevinbgreene merged commit 724cdb4 into master Sep 21, 2017

kevin-greene-ck deleted the validator branch August 31, 2018 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a validation pass to check types, field IDs and Identifiers #70

Adding a validation pass to check types, field IDs and Identifiers #70

kevinbgreene commented Sep 20, 2017

phated left a comment

phated Sep 20, 2017

kevinbgreene Sep 20, 2017

phated Sep 20, 2017

kevinbgreene Sep 20, 2017

phated Sep 20, 2017

kevinbgreene Sep 20, 2017

phated Sep 20, 2017

kevinbgreene Sep 20, 2017

phated Sep 20, 2017

kevinbgreene Sep 20, 2017

phated Sep 20, 2017

kevinbgreene Sep 20, 2017

phated Sep 20, 2017

kevinbgreene Sep 20, 2017

ian-harshman-ck Sep 20, 2017

ian-harshman-ck Sep 20, 2017

ian-harshman-ck Sep 20, 2017

Adding a validation pass to check types, field IDs and Identifiers #70

Adding a validation pass to check types, field IDs and Identifiers #70

Conversation

kevinbgreene commented Sep 20, 2017

phated 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

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

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