Add TypeChecker class. #153

matanlurey · 2017-06-18T22:26:20Z

This will be nice to have versus stuffed into the AngularDart compiler, and it doesn't feel "right" going into a library like build, which I think benefits from having a very small interface and low-threshold for changes.

Advantages of this approach

Adds an "approved" (not `/lib/src/annotations.dart') way of performing checks.
Has two backing implementations not based on dart:mirrors (for long-term safety).
Easy re-usable setup for complex compilers like inject or angular.

See type_checker_test.dart for some functional tests.

Closes #136.

thosakwe · 2017-06-18T22:55:56Z

I just saw this PR. This might also close #155 (which I just opened). I'll have to check...

kevmoo · 2017-06-19T00:23:49Z

@matanlurey I like...but!

I noticed when trying to convert the JsonSerialiableGenerator to it it blows up w/ the dynamic element – which (for some reason) has no source.

If you could – check it out. Or I'll do a follow-up. It's certainly better than the hacks I have in place to detect DateTime, Iterable, and List

matanlurey · 2017-06-19T00:32:56Z

@kevmoo Let me try converting your generator as a test case then.

kevmoo · 2017-06-19T00:34:30Z

Look at this crazy https://github.com/dart-lang/source_gen/blob/master/lib/generators/json_serializable_generator.dart#L318

…

On Sun, Jun 18, 2017 at 5:32 PM, Matan Lurey ***@***.***> wrote: @kevmoo <https://github.com/kevmoo> Let me try converting your generator as a test case then. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCihtaOiZkCMeliEoVcAZGBd7zGu_kks5sFcG4gaJpZM4N9n6Y> .

matanlurey · 2017-06-19T00:40:01Z

@kevmoo Just updated, and added annotationOf which seems to work fine:

String _fieldToJsonMapKey(String fieldName, FieldElement field) {
  const $JsonKey = const TypeChecker.fromRuntime(JsonKey);
  var jsonKey = $JsonKey.annotationOf(field);
  if (jsonKey != null) {
    var jsonName = jsonKey.getField('jsonName').toStringValue();
    return jsonName;
  }
  return fieldName;
}

kevmoo

LGTM – let's see what @jakemac53 says...

kevmoo · 2017-06-19T15:49:32Z

See broken build – Cannot resolve "asset:source_gen/lib/generators/json_serializable.dart".

Ugh...

jakemac53 · 2017-06-19T15:13:44Z

lib/src/type_checker.dart

+  /// Returns the first constant annotating [element] that is this type.
+  ///
+  /// Otherwise returns `null`.
+  DartObject annotationOf(Element element) {


Should this be firstAnnotationOf? And then possibly add annotationsOf?

jakemac53 · 2017-06-19T15:15:09Z

lib/src/type_checker.dart

+  }
+
+  /// Returns `true` if representing the exact same class as [element].
+  bool isExactly(Element element);


Should this be ClassElement?

Good question. Originally I had it, but I changed because:

Most APIs return Element, not ClassElement, and this avoids users having to cast all over.

There are other types not yet covered (like Function types) that are not classes but could be used here.

jakemac53 · 2017-06-19T15:16:32Z

lib/src/type_checker.dart

+      element is ClassElement && element.allSupertypes.any(isExactlyType);
+
+  /// Returns `true` if representing a super type of [staticType].
+  bool isSuperType(DartType staticType) => isSuperOf(staticType.element);


nit: isSuperTypeOf for consistency

jakemac53 · 2017-06-19T15:19:21Z

lib/src/type_checker.dart

+
+/// An abstraction around doing static type checking at compile/build time.
+abstract class TypeChecker {
+  const TypeChecker._();


Do you need this constructor? You should be able to delete it and remove the super constructor calls.

I could remove it, but then the other classes couldn't be const. Since I wanted that anyway, I figured making the TypeChecker class itself not extendable is probably an OK thing to do.

Ah ok just for the const, makes sense. We can always make this public later if somebody actually has a valid reason to extend it and files an issue.

jakemac53 · 2017-06-19T15:21:42Z

lib/src/type_checker.dart

+  bool isSuperOf(Element element) =>
+      isExactly(element) ||
+      element is ClassElement &&
+          element.allSupertypes.any((t) => t.element == _type.element);


Why is the logic different for checking super types? (here it checks for ClassElement equality where isExactly checks for DartType equality)

Fixed, thanks!

jakemac53 · 2017-06-19T15:26:18Z

lib/src/type_checker.dart

+          fragment: MirrorSystem.getName(mirror.simpleName),
+        );
+      default:
+        throw new StateError('Cannot resolve "$sourceUri".');


maybe add an additional note You must import the ${mirror.simpleName} class using a package: or dart: uri..

jakemac53 · 2017-06-19T15:33:42Z

lib/src/type_checker.dart

+    var sourceUri = element.source.uri;
+    switch (sourceUri.scheme) {
+      case 'dart':
+        if (sourceUri.pathSegments.isNotEmpty) {


Is it an error if this is empty? Also when is it more than one segment?

Added a comment why.

where? (maybe forgot to push a commit?)

jakemac53 · 2017-06-19T15:44:01Z

test/type_checker_test.dart

+
+    final core = resolver.getLibraryByName('dart.core');
+    staticMap = core.getType('Map').type;
+    staticMapChecker = new TypeChecker.fromStatic(staticMap);


Might as well just create this inline in the test that uses them below like the others instead of creating them for all tests (same for staticHashMapChecker).

The only reason I did this is to avoid creating a Resolver multiple times per test.

oh right, setUpAll just runs once doesn't it, sgtm

jakemac53 · 2017-06-19T15:45:40Z

test/type_checker_test.dart

+      });
+
+      test('should not be a super type of dart:core#Map', () {
+        expect(checkMap().isSuperType(staticMap), isTrue);


Shouldn't this be isFalse?

Done and fixed.

jakemac53 · 2017-06-19T15:48:06Z

test/type_checker_test.dart

+        expect(
+          checkMap().isExactlyType(staticMap),
+          isTrue,
+          reason: '$checkMap != $staticMap',


nit: no trailing comma ;) (also below).

jakemac53

Generally LGTM, but it would be good to also add tests for non-core classes if you don't mind.

jakemac53 · 2017-06-19T16:16:21Z

lib/src/type_checker.dart

+  }
+
+  /// Returns `true` if representing the exact same class as [element].
+  bool isExactly(Element element);


matanlurey · 2017-06-19T17:25:33Z

As discussed offline with @jakemac53, filed #161 to add additional test cases - need some infra build-out for pkg/build_test first.

kevmoo · 2017-06-19T19:10:00Z

@matanlurey thoughts on the broken travis bits?

matanlurey · 2017-06-19T19:13:08Z

@kevmoo Fixing now, I had some mismatch b/w asset: and package: URLs.

New functionality normalizes the two (internally). Will upload as soon as all tests pass.

kevmoo · 2017-06-19T19:45:38Z

@matanlurey for this PR or later – would be great to have a isExactlyOrIsSuperTypeOf – or similar.

See 0046bdd#diff-c4274d5939b2f855cfba15318f2d42ebR250

Having to do isExact || isSuper several places

matanlurey · 2017-06-19T19:48:19Z

@kevmoo Send a PR after I merge? I finally got this one green :P

kevmoo · 2017-06-19T19:49:52Z

Send a PR after I merge? I finally got this one green :P

Deal. 👍

* Add TypeChecker class. * Added annotationOf. * Added a fallback case for dynamic. * Address comments. * Normalize. * Dartfmt.

Add TypeChecker class.

07ebca8

matanlurey requested review from jakemac53 and kevmoo June 18, 2017 22:26

googlebot added the cla: yes label Jun 18, 2017

Added annotationOf.

bbde90b

Added a fallback case for dynamic.

c11b270

kevmoo reviewed Jun 19, 2017

View reviewed changes

jakemac53 requested changes Jun 19, 2017

View reviewed changes

Address comments.

da74b88

jakemac53 reviewed Jun 19, 2017

View reviewed changes

matanlurey added 2 commits June 19, 2017 12:13

Normalize.

71333fb

Dartfmt.

b72548d

kevmoo added a commit that referenced this pull request Jun 19, 2017

Building off #153

0046bdd

kevmoo mentioned this pull request Jun 19, 2017

Added isAssignableFrom(Type) to TypeChecker #164

Merged

jakemac53 approved these changes Jun 19, 2017

View reviewed changes

matanlurey merged commit d498497 into dart-lang:master Jun 19, 2017

matanlurey deleted the static_types branch June 19, 2017 19:53

kevmoo added a commit that referenced this pull request Jun 19, 2017

Building off #153

7706669

mosuem pushed a commit to dart-lang/build that referenced this pull request Dec 10, 2024

Add TypeChecker class. (dart-lang/source_gen#153)

c95b78e

* Add TypeChecker class. * Added annotationOf. * Added a fallback case for dynamic. * Address comments. * Normalize. * Dartfmt.

mosuem pushed a commit to dart-lang/build that referenced this pull request Dec 10, 2024

Add TypeChecker class. (dart-lang/source_gen#153)

91d9539

* Add TypeChecker class. * Added annotationOf. * Added a fallback case for dynamic. * Address comments. * Normalize. * Dartfmt.

Add TypeChecker class. #153

Add TypeChecker class. #153

Uh oh!

Conversation

matanlurey commented Jun 18, 2017

Uh oh!

thosakwe commented Jun 18, 2017

Uh oh!

kevmoo commented Jun 19, 2017

Uh oh!

matanlurey commented Jun 19, 2017

Uh oh!

kevmoo commented Jun 19, 2017 via email

Uh oh!

matanlurey commented Jun 19, 2017

Uh oh!

kevmoo left a comment

Choose a reason for hiding this comment

Uh oh!

kevmoo commented Jun 19, 2017

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!

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!

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!

jakemac53 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey commented Jun 19, 2017

Uh oh!

kevmoo commented Jun 19, 2017

Uh oh!

matanlurey commented Jun 19, 2017

Uh oh!