[dart] fix some compilation issues and added casts for analyzer implicit-casts flag

[Grohden]

Details

I've made some fixes for the dart templates, @kuhnroyal and I had some discussion on #8179.

My changes were necessary to be able to compile my personal project and also the fake petstore generated clients.

Not all changes were made to fix compile errors: I've mistakenly enabled the implicit-casts:false on the generated client and then fixed the resulting analyzer errors..

• if fixing casts 'issues' and supporting that flag is not desired I can revert my cast changes.
• if fixing casts 'issues' and supporting that flag is desired we could enable the strong mode like @kuhnroyal did in [dart][dart-dio] Enable strong-mode and strict types #8231

From the changelist listed in #8179 I've fixed:

• De-serialization of maps of maps e.g. Map<String, Map<String, String>>

(I've only fixed the compilation error, not sure if it will work properly on runtime)

Maybe I've fixed other issues.. but I'm not sure, would be glad if someone test it and confirm exactly what is still broken

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
• Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@kuhnroyal could you comment on the casts you said that were uneeded?

@swipesight @jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[kuhnroyal]

You should add a matching analysis_options.yaml as a new support template file, like it is done in the dio-generator.

[agilob]

@kuhnroyal @Grohden
what do you two think about adding something more popular and opinionated than a custom analysis? Would you add effective-dart here?

[Grohden]

@agilob would totally use effective dart, I can make the needed changes if @kuhnroyal also agrees

edit: effective could be done in another PR to not bloat this one

[kuhnroyal]

I would love to see a linter in dart/dart-io at some point.
Need to decide which makes the most sense here.
https://pub.dev/packages/effective_dart
https://pub.dev/packages/lint
https://pub.dev/packages/pedantic

I usually use lint.
And yes, should be done in another PR and added to both generators as well as to the integration tests.

[agilob]

Pedantic always makes my eyes bleed, it makes Dart have more strict syntax than Java. I personally stick with effective_dart, but I've never used lint... and I think I know why An opinionated, community-driven set of lint rules for Dart and Flutter projects. Like pedantic but stricter

[agilob]

edit: effective could be done in another PR to not bloat this one

I disagree here, since you're adding lint fixes it makes sense to add the linter too!

[Grohden]

I disagree here, since you're adding lint fixes it makes sense to add the linter too!

Somehow this PR ended up as lint fixes, but my primary problem was the Map.mapfromJson not letting me compile my project.. I've mistakenly enabled the analyzer and fixed these cast issues 😆 I can remove the implicit-casts:false and its fixes and enable it in a separated PR for the linter if that's better

[kuhnroyal]

Yes, we should have a separate PR for the strong-mode/implicit changes. Can you use the same options I used here #8231
I just checked out this branch and tried with the analysis_options from my branch and all the json as Foo casts are somehow unnecessary. So I would really love to see your analyzer options.

[agilob]

No probs, so just add the analysis.yaml so we know what the changes are related to.

[Grohden]

@kuhnroyal my analysis options for the petstore is just:

analyzer:
  strong-mode:
    implicit-casts: false

[kuhnroyal]

Ok so I see the problem now. I never disable implicit casts because that doesn't do much good.
Assigning dynamic to a String is just as safe as casting it, if it is not a String it will fail either way.

On the other hand having implicit-dynamic: false actually prevents type errors.
After realizing we are talking about different settings I have to say I am against implicit-casts: false.

[Grohden]

@kuhnroyal @agilob
I've removed the fixes related to the implicit-casts: false, I've also changed the way I deal with the Maps of maps case

After realizing we are talking about different settings I have to say I am against implicit-casts: false.

I've only learned from your comments about the implicit-dynamic: false, so agree with you and (some) of my recent changes are respecting this flag, but I'm gonna open another PR for the analysis file (and also lint fixes).

Edit: I vote for the effective_dart, I never heard or used lint... but Like pedantic but stricter doesn't sound good, I don't like pedantic rules either

[agilob]

Can you add the analysis yaml file pls?

[Grohden]

@agilob
U mean the one from #8231 or one with

analyzer:
  strong-mode:
    implicit-casts: true

(which is to explicitly accept implicit-casts)
?

[agilob]

The one that triggers warnings you fixed here

[Grohden]

@agilob file added

[Grohden]

@kuhnroyal I was bugged about why I had analyzer errors reported on the _updateParamsForAuth from the invokeAPI, it turns out that it was the implicit-casts: false that was reporting that error to me.

So just so you know, this code exemplifies a case where this flag can be helpful:

void main() {
  foo({"a", "b"});
}

void foo(Iterable<String> authNames) {
  // implicit-casts: false -> The argument type 'Iterable<String>' can't be assigned to the parameter type 'List<String>'.
  // implicit-casts: true -> ok, just runtime error :D
  bar(authNames); 
}

void bar(List<String> authNames) {
  authNames.add("bar");
}

[agilob]

// implicit-casts: false -> The argument type 'Iterable' can't be assigned to the parameter type 'List'.

Yea, that's a good fix. I'll address the bigger issue some time later, it shouldn't be List there :(

[kuhnroyal]

I agree that this is one of the few cases were implicit-cast: false would highlight the potential bug. The question is, is the overhead in all the other places worth it? I am not sure. What do you guys think?

[Grohden]

@kuhnroyal I am the "if the compiler say its ok, then its ok" guy, so after seeing the flag capturing an actual bug I'm not against it - I'm gonna keep using it in personal projects.

But I agree that having to cast every single dynamic is annoying and unnecessary, and the dart API that we have is not huge and is not expected to change very often.. so I would keep the flag (explicitly) disabled

[agilob]

would highlight the potential bug. The question is, is the overhead in all the other places worth it? I am not sure. What do you guys think?

Let's keep what we have here and address linter in another PR. I think it's fair to add the strictest linter in the generated code. I can do it after this gets in.

@kuhnroyal I am the "if the compiler say its ok, then its ok" guy,

It compiles = it goes to production!

[agilob]

But I agree that having to cast every single dynamic is annoying and unnecessary, and the dart API that we have is not huge and is not expected to change very often.. so I would keep the flag (explicitly) disabled

In case of autogenerated code I think correctness and type-safety is more important than "I dont like it has 2 casts every 5 lines in files I never modify manually".

I use effective-dart in my projects, because it doesnt put many restriction on code I write, but any autogenerated code must be absolutely safe to a high standard (openapi spec) so the strictest linter the better.

[kuhnroyal]

I agree, since it is generated we can add the casts. But they need to be the correct ones and that is not always easy to see.
For the bug that was found, most people would have just blindly added the cast as List to make it compile. It would still fail with a Set but the compiler is happy.

[wing328]

@Grohden can you please resolve the merge conflicts when you've time? Thank you.

[Grohden]

@wing328 I've merged the latest master today and regenerated the needed files..

I probably need to test if the new added anaysis_options.yaml file will affect the new (?) json serializable option... but that's gonna take time if it depends on me

[agilob]

I probably need to test if the new added anaysis_options.yaml file will affect the new (?) json serializable option...

I can look into it after this one is merged

[wing328]

@Grohden sorry do you mind resolving the merge conflicts again? We'll put other Dart PRs on hold for the time being.

[Grohden]

@wing328 no problem

I've merged today's master and re-generated all dart codes (./bin/generate-samples.sh ./bin/configs/dart*)

[wing328]

Travis CI failure not related to this PR.