google / built_value.dart

Immutable value types, enum classes, and serialization.
https://pub.dev/packages/built_value
BSD 3-Clause "New" or "Revised" License
870 stars 184 forks source link

Cast in deserialize method of generated serializer fails strong mode linting with @BuiltValue(nestedBuilders: false) #669

Closed nattyg93 closed 4 years ago

nattyg93 commented 5 years ago

I have implicit casts disabled in my analysis_options.yaml.

i.e.

analyzer:
  strong-mode:
    implicit-casts: false
    implicit-dynamic: false

I have the class below:

@BuiltValue(nestedBuilders: false)
abstract class Example implements Built<Example, ExampleBuilder> {
  static Serializer<Example> get serializer => _$exampleSerializer;

  BuiltMap<String, JsonObject> get example;

  Example._();
  factory Example([void Function(ExampleBuilder) updates]) = _$Example;
}

I get the following generated serializer, which fails linting:

class _$ExampleSerializer implements StructuredSerializer<Example> {
  @override
  final Iterable<Type> types = const [Example, _$Example];
  @override
  final String wireName = 'Example';

  @override
  Iterable serialize(Serializers serializers, Example object,
      {FullType specifiedType = FullType.unspecified}) {
    final result = <Object>[
      'example',
      serializers.serialize(object.example,
          specifiedType: const FullType(BuiltMap,
              const [const FullType(String), const FullType(JsonObject)])),
    ];

    return result;
  }

  @override
  Example deserialize(Serializers serializers, Iterable serialized,
      {FullType specifiedType = FullType.unspecified}) {
    final result = new ExampleBuilder();

    final iterator = serialized.iterator;
    while (iterator.moveNext()) {
      final key = iterator.current as String;
      iterator.moveNext();
      final dynamic value = iterator.current;
      switch (key) {
        case 'example':
          result.example = serializers.deserialize(value,
              specifiedType: const FullType(BuiltMap, const [
                const FullType(String),
                const FullType(JsonObject)
              ])) as BuiltMap;  // <---- This should be BuiltMap<String, JsonObject>
          break;
      }
    }

    return result.build();
  }
}

It's probably worth noting that it seems to only happen when using BuiltMap, BuiltList etc. If I have Map<String, JsonObject> get example; then the serializer will be generated with ... as Map<String, JsonObject>.

And it is only an issue when using @BuiltValue(nestedBuilders: false).

davidmorgan commented 5 years ago

Would BuiltMap<dynamic, dynamic> work/pass? Or BuiltMap<Object, Object>?

nattyg93 commented 5 years ago

No, I don't believe so. For the linter to be happy it needs to be a BuiltMap<String, JsonObject>.

Sorry, I should have attached the actual error message in my original post. On the line with the comment the linter complains that:

A value of type 'BuiltMap' can't be assigned to a variable of type 'BuiltMap<String, JsonObject>'.
Try changing the type of the variable, or casting the right-hand type to 'BuiltMap<String, JsonObject>'.dart(invalid_assignment)
nattyg93 commented 5 years ago

Sorry, I think I may have misunderstood your comment. Do you mean changing the line BuiltMap<String, JsonObject> get example; to BuiltMap<Object, Object> get example;? Changing the definition does solve the issue of the linter complaining.

davidmorgan commented 5 years ago

No, I meant changing the cast, i.e. as BuiltMap<dynamic, dynamic> ...

nattyg93 commented 5 years ago

In that case, no. The linter expects the cast to be as BuiltMap<String, JsonObject>. I suspect that under strong mode, the whole generic type will need to be fully expanded.

I doubt it changes things, but FYI, I did simplify this example. My actual code is a BuiltMap<String, BuiltMap<String, JsonObject>>, but the result is the same regardless.

davidmorgan commented 5 years ago

Thanks. I'll try to take a look; I don't have a lot of bandwidth for built_value work right now though. I suppose the quickest workaround would be to add this error to the list of '// ignore's :/

nattyg93 commented 5 years ago

Honestly, adding it to the ignore's would make me happy. Do you know what the ignore would be? I did have a cursory look in the past for it.

davidmorgan commented 5 years ago

If you ask the analyzer for csv format output with --format=machine then the output will include the error code.

nattyg93 commented 5 years ago

I did not know you could do that. Thank you!

The error is invalid_assignment, it feels a bit risky to be ignoring that, but I've gone ahead and added it in.

Shall I leave this issue open until a proper fix is added?

davidmorgan commented 5 years ago

Yes, please leave open for a proper fix. Thanks :)

knaeckeKami commented 4 years ago

So, I'm running into this, too and did a little bit of digging. As far as I am aware, there is no // ignore for strong mode settings, this is only for linters.

I have code like

BuiltMap<String, ProductDetails> get productDetails;

and the generated code for this is:

 result.productDetails = serializers.deserialize(value,
              specifiedType: const FullType(BuiltMap, const [
                const FullType(String),
                const FullType(ProductDetails)
              ])) as BuiltMap<dynamic, dynamic>;

which gives the error:

error: A value of type 'BuiltMap' can't be assigned to a variable of type 'BuiltMap<String, ProductDetails>'. (invalid_assignment at ...)

Normally, this would be fixed by adding:

analyzer:
  exclude: [build/**, lib/**/*.g.dart]

in your analysis_options.yaml. And it does, as this makes the program compile again. But there is this bug of the dartanalyzer: https://github.com/dart-lang/sdk/issues/25551

Which causes this to still show up as errors in IDEs (if you happen open the .g.dart file). Android Studio also gives you warnings on Hot Reloading if this happens, and the only way to fix this is to close the .g.dart file, close android studio and open it again.

Now, obviously this should be fixed in the dartanalyzer. However, this bug has been open for almost three years and is unlikely to be fixed soon.

So I think in the meantime, it would be nice if built_value would implement code compliant with implicit-dynamic: false.

I only run into this in BuiltLists and BuiltMaps, and only when using no nested builders. Changing the casts from as BuiltMap<dynamic, dynamic> to as BuiltMap<Actualtype1, Actualtype2>; should fix this.

knaeckeKami commented 4 years ago

It seems like the culprit for this is this piece of code:

    // For built collections we can cast to the dynamic type when deserializing,
    // instead of the actual generic type. This is because the `replace` method
    // checks the generic type and copies if needed.
    String generics;
    if (topLevel && DartTypes.isBuiltCollectionTypeName(bareType)) {
      if (bareType == 'BuiltList' || bareType == 'BuiltSet') {
        generics = 'dynamic';
      } else if (bareType == 'BuiltMap' ||
          bareType == 'BuiltListMultimap' ||
          bareType == 'BuiltSetMultimap') {
        generics = 'dynamic, dynamic';
      } else {
        throw UnsupportedError('Bare type is a built_collection type, but not '
            'one of the known built_collection types: $bareType.');
      }
    } else {
      generics = _getGenerics(type);
    }

on serializer_source_field.dart:199

Removing the special case for collections and just doing

String generics = _getGenerics(type);

seems to generate code compliant with implicit-dynamic: false. Is there are reason for this check? Why not just use the correct generic values always?

davidmorgan commented 4 years ago

Interesting, thanks. I will try that.