[pigeon] Added non-null fields by gaaclarke · Pull Request #549 · flutter/packages

gaaclarke · 2022-01-19T01:21:02Z

Implements non-null fields to data classes in the front end parser and the dart and java generator

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2022-01-20T00:43:07Z

I've merged the java implementation PR into this PR since it hasn't been reviewed yet and may be easier to review this way.

gaaclarke · 2022-01-22T00:11:54Z

Okay, I pushed objc support too since I was still waiting for a review. I promise to add no more features haha.

stuartmorgan-g · 2022-01-26T19:38:58Z

packages/pigeon/bin/run_tests.dart

Could you add a comment to this method? I have no idea what "jobs" are.

I still had to read the implementation to know what this meant, since without much context "input" and "output" are extremely generic terms.

How about: "Generates multiple dart files via pigeon invocations based on the jobs [...] the format of (key: pigeon input file, value: pigeon dart output file)."

stuartmorgan-g · 2022-01-26T19:40:49Z

packages/pigeon/CHANGELOG.md

Could you update the README section about NNBD to reflect the state with this change?

stuartmorgan-g · 2022-01-26T19:44:18Z

packages/pigeon/lib/dart_generator.dart

Why not (pigeonMap[\'${field.name}\']$unwrapOperator as $genericdType)? It's more succinct, and I'm not aware of a reason to prefer casting first.

yea that should world, done

stuartmorgan-g · 2022-01-26T19:46:00Z

packages/pigeon/lib/java_generator.dart

nit: s/,/;/

(I would also change to "Constructor is private [...]"; it's only three more characters, and much easier to read things that don't missing verbs 🙂 )

packages/pigeon/lib/objc_generator.dart

stuartmorgan-g · 2022-01-26T19:55:28Z

packages/pigeon/lib/objc_generator.dart

I know this is pre-existing, but I didn't notice before (or didn't review this section originally?): why is there casting happening here? Storing NSNull in a typed field abuses the type system. That should have been screened out earlier.

Thats the convention that the standard method codec uses: https://github.com/flutter/engine/blob/ebdeec054a736a149880a7bbb9359ce3e3c80f4e/shell/platform/darwin/common/framework/Source/FlutterStandardCodec.mm#L472

That's not a codec convention, that's how you store conceptually null values in a dictionary. I'm not asking why there are NSNulls in the dictionary though; there's no type violation there because the values in an NSDictionary are, by default (as in that code), NSObjects, and NSNull is an NSObject. I'm asking why field, whose type is presumably not NSObject, has an NSNull stored in it.

The proper way to extract a possibly-null value from a dictionary into a typed variable is to check it before making what would be a type-violating assignment. I.e.:

NSString *foo = myDict['fooKey'] == [NSNull null] ? nil : myDict['fooKey']; if (foo != nil) { ... }

is perfectly valid code. But

NSString *foo = myDict['fooKey']; if (foo != nil && foo != [NSNull null]) { .. }

is not, because there is a codepath there where the thing foo is pointing to is an unrelated type. (And when I say it's not valid code, I mean that it's code we should not write/generate because it violates the type contract; I know it will technically work because it's a dynamic runtime.)

I see, the reason I've done the latter is to avoid multiple lookups and to avoid introducing more naming. When you are doing codegen anytime you create a name (variable, function, class) you run a risk of colliding, so I avoid it. Other metaprogramming systems solve this with hygienic macros or something like common-lisp's gensym. We don't have those available to us so I sometimes do things to avoid introducing new symbols.

Here is some generate code from that area:

// header @interface SearchRequest : NSObject /// `init` unavailable to enforce nonnull fields, see the `make` class method. - (instancetype)init NS_UNAVAILABLE; + (instancetype)makeWithQuery:(NSString *)query; @property(nonatomic, copy) NSString * query; @end // source @interface SearchRequest () + (SearchRequest *)fromMap:(NSDictionary *)dict; - (NSDictionary *)toMap; @end @implementation SearchRequest + (instancetype)makeWithQuery:(NSString *)query { SearchRequest* pigeonResult = [[SearchRequest alloc] init]; pigeonResult.query = query; return pigeonResult; } + (SearchRequest *)fromMap:(NSDictionary *)dict { SearchRequest *pigeonResult = [[SearchRequest alloc] init]; pigeonResult.query = dict[@"query"]; NSAssert((NSNull *)pigeonResult.query != [NSNull null], @""); return pigeonResult; } - (NSDictionary *)toMap { return [NSDictionary dictionaryWithObjectsAndKeys:(self.query ? self.query : [NSNull null]), @"query", nil]; } @end

So, any variable name we chose there for a local variable could collide with with the field names. We can mitigate that by adding a "pigeon" prefix to the variable names. Like I said over vc, I just wanted to avoid any issue.

We should definitely fix that using prefixed variables; we shouldn't generated code that's abusing the type system. It can be a follow-up since it's pre-existing.

stuartmorgan-g · 2022-01-26T19:59:51Z

packages/pigeon/test/objc_generator_test.dart

Is this space-on-both-sides-of-* new? I thought we resolved that previously, but maybe I'm misremembering. We really shouldn't be generating that.

It's not new but we talked about this in the past, here is the line that is generating it:

indent.writeln( '@property(nonatomic, $propertyType$nullability) ${hostDatatype.datatype} ${field.name};');

We'd have to do some conditional logic to add a space or not depending if the datatype ends with a "*". I'd rather not add that complexity.

gaaclarke · 2022-01-26T23:37:29Z

@stuartmorgan here this should help, I've added a flag that will give you the information you want to help you understand the code (and to help developers including myself).

invocation:
pub run pigeon --input pigeons/non_null_fields.dart --objc_source_out stdout --one_language --debug_generators

output:

// Autogenerated from Pigeon (v1.0.16), do not edit directly. //#2      generateObjcSource (package:pigeon/objc_generator.dart:702:10)
// See also: https://pub.dev/packages/pigeon //#2      generateObjcSource (package:pigeon/objc_generator.dart:703:10)
#import "null" //#2      generateObjcSource (package:pigeon/objc_generator.dart:704:10)
#import <Flutter/Flutter.h> //#2      generateObjcSource (package:pigeon/objc_generator.dart:705:10)
 //#2      generateObjcSource (package:pigeon/objc_generator.dart:706:10)
#if !__has_feature(objc_arc) //#2      generateObjcSource (package:pigeon/objc_generator.dart:708:10)
#error File requires ARC to be enabled. //#2      generateObjcSource (package:pigeon/objc_generator.dart:709:10)
#endif //#2      generateObjcSource (package:pigeon/objc_generator.dart:710:10)
 //#2      generateObjcSource (package:pigeon/objc_generator.dart:711:10)
static NSDictionary<NSString *, id> *wrapResult(id result, FlutterError *error) { //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
  NSDictionary *errorDict = (NSDictionary *)[NSNull null]; //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
  if (error) { //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
    errorDict = @{ //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
        @"code": (error.code ? error.code : [NSNull null]), //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
        @"message": (error.message ? error.message : [NSNull null]), //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
        @"details": (error.details ? error.details : [NSNull null]), //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
        }; //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
  } //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
  return @{ //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
      @"result": (result ? result : [NSNull null]), //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
      @"error": errorDict, //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
      }; //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
} //#3      generateObjcSource (package:pigeon/objc_generator.dart:713:10)
 //#2      generateObjcSource (package:pigeon/objc_generator.dart:728:10)
@interface SearchRequest () //#2      generateObjcSource (package:pigeon/objc_generator.dart:732:12)
+ (SearchRequest *)fromMap:(NSDictionary *)dict; //#2      generateObjcSource (package:pigeon/objc_generator.dart:733:12)
- (NSDictionary *)toMap; //#2      generateObjcSource (package:pigeon/objc_generator.dart:734:12)
@end //#2      generateObjcSource (package:pigeon/objc_generator.dart:735:12)
@interface SearchReply () //#2      generateObjcSource (package:pigeon/objc_generator.dart:732:12)
+ (SearchReply *)fromMap:(NSDictionary *)dict; //#2      generateObjcSource (package:pigeon/objc_generator.dart:733:12)
- (NSDictionary *)toMap; //#2      generateObjcSource (package:pigeon/objc_generator.dart:734:12)
...

gaaclarke · 2022-01-28T22:38:23Z

@stuartmorgan Friendly ping: all comments are addressed. Please check that everything is satisfactory, hopefully the new comments make everything easier now.

stuartmorgan-g

LGTM with some small refactorings to extract code where you are adding complexity to already-complex functions.

stuartmorgan-g · 2022-01-31T19:45:20Z

packages/pigeon/bin/run_tests.dart

I still had to read the implementation to know what this meant, since without much context "input" and "output" are extremely generic terms.

How about: "Generates multiple dart files via pigeon invocations based on the jobs [...] the format of (key: pigeon input file, value: pigeon dart output file)."

stuartmorgan-g · 2022-01-31T19:48:43Z

packages/pigeon/lib/dart_generator.dart

The comments you've added to the methods are incredibly helpful. Unfortunately this new code is in a mega-method that doesn't have that kind of example.

Given that this is a) huge and b) very nested could you start breaking pieces out? This PR doesn't have to do it all retroactively, but the fact that you have a named subsection here suggests this could easily be a new method (with example code), and over time more of the method could be refactored that way.

Done, used nested functions to introduce more structure and a high level view of functions

stuartmorgan-g · 2022-01-31T19:49:29Z

packages/pigeon/lib/dart_generator.dart

This whole loop body seems like another good candidate for extraction.

Done (inner function).

stuartmorgan-g · 2022-01-31T19:51:42Z

packages/pigeon/lib/java_generator.dart

Same thing here; the constructor/builder seems like a great thing to extract from this large-and-growing method.

Done (inner functions)

stuartmorgan-g · 2022-01-31T19:52:55Z

packages/pigeon/lib/java_generator.dart

As does some section of this (maybe the loop body, or maybe the bit before plus the loop; it's hard to tell—thus the comment :)—what the coherent sections are to see where best to cut it.)

stuartmorgan-g · 2022-01-31T19:56:42Z

packages/pigeon/lib/objc_generator.dart

We should definitely fix that using prefixed variables; we shouldn't generated code that's abusing the type system. It can be a follow-up since it's pre-existing.

github-actions bot added the p: pigeon label Jan 19, 2022

gaaclarke changed the title ~~[pigeon] Added non-null types for dart generator~~ [pigeon] Added non-null fields for dart generator and front-end Jan 19, 2022

gaaclarke force-pushed the non-null-fields-dart branch 4 times, most recently from 0d96ed9 to e69d194 Compare January 19, 2022 18:02

gaaclarke marked this pull request as ready for review January 19, 2022 18:33

gaaclarke requested a review from stuartmorgan-g January 19, 2022 18:33

gaaclarke changed the title ~~[pigeon] Added non-null fields for dart generator and front-end~~ [pigeon] Added non-null fields for dart, java and the front-end Jan 20, 2022

gaaclarke force-pushed the non-null-fields-dart branch from d5d0672 to 4129225 Compare January 20, 2022 00:51

gaaclarke changed the title ~~[pigeon] Added non-null fields for dart, java and the front-end~~ [pigeon] Added non-null fields Jan 22, 2022

stuartmorgan-g requested changes Jan 26, 2022

View reviewed changes

gaaclarke requested a review from stuartmorgan-g January 27, 2022 00:08

gaaclarke force-pushed the non-null-fields-dart branch 4 times, most recently from 22447f1 to ad1b0ed Compare January 27, 2022 22:37

stuartmorgan-g approved these changes Jan 31, 2022

View reviewed changes

gaaclarke added 8 commits January 31, 2022 17:06

[pigeon] Added non-null types for dart generator

bace56a

[pigeon] Added support for non-null fields in java data classes

8152b68

added objc support for non-null-fields

b4b3f41

updated version

de9d289

added objc_generator test beyond the platform tests

d0846e3

stuarts feedback 1

9add911

added ability to print out debugging information about generators

6d53f1b

updated readme

a58bd5f

gaaclarke added 6 commits January 31, 2022 17:08

serialized dart code generation to work around dart bug for now

055f654

backfilled objc docstrings

52da433

added dart docstrings

049053b

added docstrings for java

bb6bfa0

switched name of local variable to avoid conflicts

d8afee5

stuarts feedback

33bbf86

gaaclarke force-pushed the non-null-fields-dart branch from 9aab6f3 to 33bbf86 Compare February 1, 2022 01:09

formatter

74cb6fe

gaaclarke added the waiting for tree to go green label Feb 1, 2022

gaaclarke merged commit 5e74e31 into flutter:main Feb 1, 2022

stuartmorgan-g mentioned this pull request Mar 15, 2022

[pigeon] Adds non-null object fields #1191

Closed

11 tasks

Conversation

gaaclarke commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gaaclarke commented Jan 20, 2022

Uh oh!

gaaclarke commented Jan 22, 2022

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jan 27, 2022 • 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!

gaaclarke Jan 27, 2022 • 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!

gaaclarke commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke commented Jan 28, 2022

Uh oh!

stuartmorgan-g left a comment

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!

stuartmorgan-g Jan 31, 2022 • 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!

gaaclarke commented Jan 19, 2022 •

edited

Loading

stuartmorgan-g Jan 27, 2022 •

edited

Loading

gaaclarke Jan 27, 2022 •

edited

Loading

gaaclarke commented Jan 26, 2022 •

edited

Loading

stuartmorgan-g Jan 31, 2022 •

edited

Loading