Fix Swift codegen escaping for identifiers, types, and strings #1515

lilyball · 2019-09-09T07:55:00Z

The source generator now operates using a string wrapper type called SwiftSource. Identifiers are escaped when converting into SwiftSource values, with an alternative method for producing escaped string literals. An escape hatch is provided for disabling escapes, but the default behavior is to escape all identifiers in the dynamic input.

Most SwiftSource values are produced by a tagged template literal, which allows for easy mixing of string literals containing Swift keywords and dynamic input that needs escaping.

As part of this, rewrite string escaping such that it actually escapes string contents properly. This required fixing a test that had a bad spec enforcing broken behavior.

Fixes apollographql/apollo-ios#193.
Fixes apollographql/apollo-ios#752.

This PR isn't quite done yet. I haven't updated the changelog, and I also haven't added tests for apollographql/apollo-ios#193 that fail without this change. I'm submitting this now because it's almost 1AM and I'm going to bed, and I'd like to get some eyes on it.

TODO:

Update CHANGELOG.md* with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

apollo-cla · 2019-09-09T07:55:02Z

@lilyball: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

designatednerd

This is awesome, thanks so much for contributing!

Most of my comments are questions because my grip on TypeScript is a little tenuous, but overall this is going to be an enormous help.

packages/apollo-codegen-swift/src/__tests__/__snapshots__/codeGeneration.ts.snap

packages/apollo-codegen-swift/src/codeGeneration.ts

designatednerd · 2019-09-09T19:08:38Z

packages/apollo-codegen-swift/src/language.ts

+        .join(" ");
+    }
+    return new SwiftSource(
+      `"${string.replace(/[\0\\\t\n\r"]/g, c => {


Would you mind throwing in a comment for the regex-impared explaining what's being replaced here?

To be honest, this is a pretty simple regex. It's just matching any of 6 characters. I suppose I could say something like // Match any character that needs escaping in Swift strings but I thought that was inferable from context.

It is, but I think maybe what threw me is I'm not really sure what's being escaped with \0, or why a single quote isn't being escaped. The other bits are pretty straightforward.

Technically all I really need to escape is \n, \r, ", and \\, but I included \0 (the NUL character) and \t because Swift has defined escapes for those and that makes the source easier to read. Single quote isn't escaped because it's not a special character in a Swift string literal.

OK - maybe if there's someplace where those are defined it might be worth throwing in a comment link to it. Otherwise, this is fine.

Good idea, I can put in a link to the Swift language reference.

packages/apollo-codegen-swift/src/language.ts

designatednerd · 2019-09-09T19:11:16Z

packages/apollo-codegen-swift/src/language.ts

+   * Concatenates multiple `SwiftSource`s together.
+   */
+  concat(...sources: SwiftSource[]): SwiftSource {
+    // Documentation says + is faster than String.concat, so let's use that


Moderately less silly question: Would this work with Typescript reduce, or is that only something that works in Swift?

This absolutely could be Array.reduce and I will make that change. I'm actually not normally a JavaScript/TypeScript programmer so I don't know offhand what functional methods are in the standard library.

Yeah neither am I, clearly.

designatednerd · 2019-09-09T19:12:33Z

packages/apollo-codegen-swift/src/language.ts

+    // Disable trimming if the string contains """ as this means we're probably printing an
+    // operation definition where trimming is destructive.
+    this.printOnNewline(
+      SwiftSource.string(string, /* trim */ !string.includes('"""'))


the inline comment here is to indicate what that parameter is, correct?

Yep. Would you prefer I remove it, or use a different style?

Nah, I just hadn't seen that particular syntax before

lilyball · 2019-09-10T04:07:59Z

Force-pushed to fix the flagged issues (including renaming elt to value), ~~though it looks like there's now a conflict.~~

Pushed again to resolve the conflict.

When the GraphQL field name `self` is used, the synthesized type name `Self` matches a reserved keyword and isn't being properly escaped. This test currently fails.

For example, the type `Self?` needs to escape the identifier `Self`.

This allows for replacing the `string` type given to `print` and `printOnNewline` with a new type, as long as it responds to `toString()`. The idea here is that languages can define their own source types and use tagged template literals to implement automatic escaping as needed.

The source generator now operates using a string wrapper type called `SwiftSource`. Identifiers are escaped when converting into `SwiftSource` values, with an alternative method for producing escaped string literals. An escape hatch is provided for disabling escapes, but the default behavior is to escape all identifiers in the dynamic input. Most `SwiftSource` values are produced by a tagged template literal, which allows for easy mixing of string literals containing Swift keywords and dynamic input that needs escaping. As part of this, rewrite string escaping such that it actually escapes string contents properly. This required fixing a test that had a bad spec enforcing broken behavior. This commit makes the test added earlier pass. Fixes apollographql/apollo-ios#193. Fixes apollographql/apollo-ios#752.

lilyball · 2019-09-10T04:35:45Z

Updated again, this time with a test for unescaped types (similar to what was encountered in apollographql/apollo-ios#193), as well as the changelog. This PR should now be good to go.

designatednerd

Looks great - I'll need to test it against our main repo this afternoon before I merge it to see if this should be a minor or patch release on the iOS SDK.

designatednerd

Looks like this'll go out in a patch. Woot!

designatednerd · 2019-09-10T20:01:11Z

@JakeDawkins Can you confirm the failing circle tests are an issue on our end? It certainly looks like it with the error › "Loading schema for engine": Error: ENGINE_API_KEY not found, but wanted to double check before merging this

designatednerd · 2019-09-11T14:44:21Z

Failing checks are due to this coming from a fork - tooling team is on it. Merging this!

lilyball · 2019-09-14T06:08:02Z

Oh good, now that this has been released, I can state that it did not introduce any performance regressions, which I was afraid it might (due to replacing strings with objects). But running apollo@2.18.0 against my codebase takes approximately the same amount of time as apollo@2.18.3 (that time being ~51 seconds, which is rather unfortunate).

designatednerd self-assigned this Sep 9, 2019

designatednerd added 🐦 component - swift 🤖 component - codegen related to the codegen core packages labels Sep 9, 2019

designatednerd reviewed Sep 9, 2019

View reviewed changes

lilyball force-pushed the swift-identifier-escaping branch from 7a8eadd to c147a13 Compare September 10, 2019 04:05

lilyball force-pushed the swift-identifier-escaping branch from c147a13 to 54c384b Compare September 10, 2019 04:13

lilyball added 5 commits September 9, 2019 21:31

swift: Add failing test for missing type name escaping

502fb3c

When the GraphQL field name `self` is used, the synthesized type name `Self` matches a reserved keyword and isn't being properly escaped. This test currently fails.

swift: escape identifiers within a more complex type

ea08531

For example, the type `Self?` needs to escape the identifier `Self`.

swift: Add documentation to reserved keywords

b9a72a3

lilyball force-pushed the swift-identifier-escaping branch from 54c384b to b9a72a3 Compare September 10, 2019 04:35

designatednerd reviewed Sep 10, 2019

View reviewed changes

designatednerd approved these changes Sep 10, 2019

View reviewed changes

designatednerd merged commit 7447b60 into apollographql:master Sep 11, 2019

designatednerd mentioned this pull request Sep 11, 2019

Fix for swift 5.1 compiler warning when type is an enum with a none case #1482

Merged

lilyball deleted the swift-identifier-escaping branch September 11, 2019 18:43

lilyball mentioned this pull request Sep 14, 2019

swift: field of type self conflicts with implicit self in initializer #1530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Swift codegen escaping for identifiers, types, and strings #1515

Fix Swift codegen escaping for identifiers, types, and strings #1515

lilyball commented Sep 9, 2019 •

edited

Loading

apollo-cla commented Sep 9, 2019

designatednerd left a comment

designatednerd Sep 9, 2019

lilyball Sep 9, 2019

designatednerd Sep 9, 2019

lilyball Sep 9, 2019

designatednerd Sep 9, 2019

lilyball Sep 9, 2019

designatednerd Sep 9, 2019

lilyball Sep 9, 2019

designatednerd Sep 9, 2019

designatednerd Sep 9, 2019

lilyball Sep 9, 2019

designatednerd Sep 9, 2019

lilyball commented Sep 10, 2019 •

edited

Loading

lilyball commented Sep 10, 2019

designatednerd left a comment

designatednerd left a comment

designatednerd commented Sep 10, 2019

designatednerd commented Sep 11, 2019

lilyball commented Sep 14, 2019

Fix Swift codegen escaping for identifiers, types, and strings #1515

Fix Swift codegen escaping for identifiers, types, and strings #1515

Conversation

lilyball commented Sep 9, 2019 • edited Loading

apollo-cla commented Sep 9, 2019

designatednerd 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

lilyball commented Sep 10, 2019 • edited Loading

lilyball commented Sep 10, 2019

designatednerd left a comment

Choose a reason for hiding this comment

designatednerd left a comment

Choose a reason for hiding this comment

designatednerd commented Sep 10, 2019

designatednerd commented Sep 11, 2019

lilyball commented Sep 14, 2019

lilyball commented Sep 9, 2019 •

edited

Loading

lilyball commented Sep 10, 2019 •

edited

Loading