Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lists for union types have missing type in generated code #89

Closed
maks opened this issue Mar 5, 2020 · 17 comments · Fixed by #204
Closed

Lists for union types have missing type in generated code #89

maks opened this issue Mar 5, 2020 · 17 comments · Fixed by #204
Labels
bug Something isn't working

Comments

@maks
Copy link
Contributor

maks commented Mar 5, 2020

Given a model class which has a list of a "union subtype":

@freezed
abstract class FApp with _$FApp {
  factory FApp({Header head, List<Page> pages}) = _FApp;
}

where the union is:

@freezed
abstract class WidgetType with _$WidgetType {
  const factory WidgetType.page({Body body, Header header}) = Page;
  const factory WidgetType.body() = Body;
  const factory WidgetType.header({String title}) = Header;
}

the type for the list in the generated code is dynamic instead of the actual type:

mixin _$FApp {
  Header get head;
  List<dynamic> get pages;

  FApp copyWith({Header head, List<dynamic> pages});
}

but in that generated code I expected page getter to be List<Page> not List<dynamic>.

It all works fine if I use List<WidgetType> but I really want to ensure I'm getting a List of Pages and not just any WidgetType

I've got a repo with the full demo code

@rrousselGit
Copy link
Owner

Do you still have the issue if you extract FApp in a different file?

@rrousselGit rrousselGit added the bug Something isn't working label Mar 5, 2020
@maks
Copy link
Contributor Author

maks commented Mar 5, 2020

Sorry I didn't explain it well, in my example repo FApp is in a separate file.

@rrousselGit
Copy link
Owner

This was fixed as part of 10.0.3 but I forgot to close the issue

@maks
Copy link
Contributor Author

maks commented Jun 9, 2020

@rrousselGit I'm sorry if I'm doing something wrong, but with my demo repo that I tested with using freezed 0.10.9 now and I am still seeing the same problem, the generated code still has:

mixin _$FApp {
  Header get head;
  List<dynamic> get pages;

  $FAppCopyWith<FApp> get copyWith;
}

so still a dynamic List instead of the actual type from the freezed "union" that I defined 😞

@rrousselGit
Copy link
Owner

What is Page then?

@maks
Copy link
Contributor Author

maks commented Jun 9, 2020

Sorry, Page is from the "union" I have:

import 'package:freezed_annotation/freezed_annotation.dart';

part 'widget_type.freezed.dart';

@freezed
abstract class WidgetType with _$WidgetType {
  const factory WidgetType.page({Body body, Header header}) = Page;
  const factory WidgetType.body() = Body;
  const factory WidgetType.header({String title}) = Header;
}

@rrousselGit
Copy link
Owner

Oh I didn't realize, my bad.

Indeed this is not supported.

@rrousselGit rrousselGit reopened this Jun 9, 2020
@maks
Copy link
Contributor Author

maks commented Jun 9, 2020

@rrousselGit I would really like to have this for a project I'm working on - is this something I could help to add to freezed or is there something fundamental that would stop freezed being able to support this?

@rrousselGit
Copy link
Owner

This is a bit difficult to support, as analyzer doesn't give the name of the type if it fails to resolve.
So Freezed never knows that it's a List<Page>, it receives List<dynamic> directly.

A first step would be to ask the Dart team to add the name of the type that failed to resolve.

@rrousselGit rrousselGit added duplicate This issue or pull request already exists blocked:analyzer and removed duplicate This issue or pull request already exists labels Jun 9, 2020
@maks
Copy link
Contributor Author

maks commented Jun 10, 2020

@rrousselGit I've started having a look and try to figure out what's happening. As you pointed out, freezed is just using what its given, I can see by setting a breakpoint inside ParserGenerator.generate() at line 29 that the LibraryReader object that parseElement() extracts a List<dynamic> for the pages parameter.

But my suspicion now is that this is actually something to do with how source_gen is working rather than the analyser as the first non generic parameter, header does get assigned the correct Header type which also comes out of the same freezed generated file as the Pages type that is supposed to be assigned to the List parameter.

I'll keeping digging down this rabbit hole and see if I can find where this is being processed inside source_gen...

@rrousselGit
Copy link
Owner

rrousselGit commented Jun 10, 2020 via email

@maks
Copy link
Contributor Author

maks commented Jun 10, 2020

That's what I thought at first too, but it works fine for the first parameter of FApp, which is Header type which is also a class generated by freezed: https://github.com/maks/esky/blob/master/lib/src/widget_type.dart

@maks
Copy link
Contributor Author

maks commented Jun 10, 2020

I did also think that it might be due to an existing open issue in source_gen around depending on generated classes but it doesn't seem to be the case as per my last comment, the generated Header type from the same freezed generated file is correctly handled.

@rrousselGit
Copy link
Owner

rrousselGit commented Jun 10, 2020

Header works because Freezed does a manual parsing in this scenario

When an object is analyzed as dynamic, Freezed does an extra parsing to try and extract the type name.

But with List<Header> that wouldn't work as we're not receiving dynamic but List<dynamic>, which is a lot more complex to handle.

@maks
Copy link
Contributor Author

maks commented Jun 10, 2020

Ah ok, sorry @rrousselGit I think I am beginning to understand now.
Oh this is super annoying! I can see the actual type stashed away inside the DefaultParameterElementImpl which is what the constructor.parameters objects actually are, but looks like there is no way to get at the property as the ParameterElement class "interface" doesn't expose any of it 😞

Screenshot 2020-06-10 at 18 25 24

@maks
Copy link
Contributor Author

maks commented Jun 11, 2020

@rrousselGit so I read through your code some more and once I figured out how it did the extra parsing, I had a basic go at extending your extra parsing to cope with generics which refer to freezed generated code classes, though I think most use cases would just be for List and Map:

maks@82b757a

It seems to work in my initial manual testing.

If this is something that you'd be interested in using? I can add some unit tests for it and check my changes don't break existing tests.

@rrousselGit
Copy link
Owner

For sure. Feel free to make a pull request 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants