Skip to content

Map literal type arguments sometimes aren't inferred with --no-implicit-dynamic #31013

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

Closed
nex3 opened this issue Oct 5, 2017 · 22 comments
Closed
Labels
legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@nex3
Copy link
Member

nex3 commented Oct 5, 2017

If I analyze the following code with --no-implicit-dynamic:

void fn(Map<dynamic, dynamic> map) {}

void main() {
  fn({1: "foo"});
}

I get this error:

  error • Missing type arguments for map literal at test.dart:4:6 • strong_mode_implicit_dynamic_map_literal

I'd expect the map's type arguments to be inferred from the values I used, or maybe to be inferred as dynamic since I'm passing it to fn(); either way, I don't expect an error.

@nex3 nex3 added analyzer-strong-mode type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 5, 2017
@anders-sandholm anders-sandholm added the legacy-area-analyzer Use area-devexp instead. label Oct 5, 2017
@Porl91
Copy link

Porl91 commented Dec 27, 2017

Ran into this recently. It seems to be related to the arguments being the dynamic type.

@bwilkerson
Copy link
Member

@stereotype441 Will this be covered by the inference back-porting that you're working on?

@stereotype441
Copy link
Member

@bwilkerson it's not directly covered by my inference back-porting work, but since it's an inference bug, I'm happy to take a look. I've added it to my list and I'll try to get to it in the next week.

@stereotype441
Copy link
Member

On second thought, isn't this the intended behavior? The idea of --no-implicit-dynamic (as I understand it) is to produce an error if type inference infers dynamic somewhere, as a way of hinting to the user that they may want to consider specifying a more precise type. That's precisely what's happening here, so I don't think it's a bug at all. (Note that if I remove the --no-implicit-dynamic option, or change the map literal to <dynamic, dynamic>{1: "foo"}, or change fn's argument type to Map<int, String>, the error goes away). @bwilkerson can you confirm that this is working as intended?

@bwilkerson
Copy link
Member

As far as I know, you're right about the intent for --no-implicit-dynamic, so it appears to be working as defined.

But then the question becomes whether it's useful to warn users in all cases, such as when we're inferring dynamic because of an explicit dynamic. Perhaps the behavior of the flag needs to be refined.

@matanlurey
Copy link
Contributor

I think we need to revisit this flag (and related ones, like --no-implicit-casts) later in 2018 (after Dart 2) and have some unified guidelines. The current implementation is helpful, but as we can see here it's not ideal for many use cases.

/cc @leafpetersen @vsmenon @jmesserly @bwilkerson

Would you be supportive of opening some meta issues ("post Dart2: Revisit --no-implicit-casts & --no-implicit-dynamic"), and then closing all issues related to this flag today as WAI? We should make it clear these flags are best effort, and not currently "production" supported as we push towards Dart 2/strong-mode.

@lrhn
Copy link
Member

lrhn commented Feb 5, 2018

I agree with Brian (propagating an explicit dynamic doesn't feel the same as inferring a dynamic from lack of information), and I would not actually think that this was an "implicit dynamic" if I wrote the code myself.

@leafpetersen
Copy link
Member

Yes, this isn't an inference bug. I think the general consensus is that the behavior of this flag should change as suggested above, but this isn't on the critical path right now.

@eernstg
Copy link
Member

eernstg commented Feb 5, 2018

tl;dr Keep flagging this implicit dynamism; dynamic may be erased here later on rd;lt

There's an inherent ambiguity in a declaration like void fn(Map<dynamic, dynamic> map) {}: The first thing to note is that this is a map from a top type to a top type, and by generic covariance this means that clients are allowed to pass any Map to fn. When we wish to know which arguments are appropriate this is all that matters, all top types work the same.

Zooming in on the details, we can see that the top type is dynamic (rather than Object or void), and that matters in two ways:

  • For the client, i.e., at a call site, the type annotation on the formal parameter map emerges (if statically known) as the type-from-context which may affect type inference on the actual argument.

  • For the body of fn, the use of dynamic in the type annotation on map has implications for the treatment of expressions containing map.

It's not at all obvious to me that the perceived needs of the client and the concrete needs felt by the developer who is writing the body will coincide. For instance, it may well be convenient for the developer who writes the body to be able to call arbitrary methods on the values retrieved from the Map, so the developer would use dynamic rather than another top type, but then all the clients may actually be negatively affected by this implementation-driven choice, simply because their actual arguments will be "somewhat dynamic" for no good reason. I tend to agree with Paul that this is an implicit dynamic which should be flagged by --no-implicit-dynamic.

However, we can use the variance of each occurrence of a top type to determine whether it's significant for the client respectively for an associated body which top type it is.

I've suggested that we should erase all top types to Object when they occur in contravariant locations of a callee. For instance, a declaration like fn would then look like void fn(Map<Object, Object> map) for clients; the "spuriously dynamic" problem which is the topic of this issue would then disappear.

In a more general setting, covariant locations are significant for the client, and contravariant locations are significant for an associated body. We've already seen that the choice among top types in the simplest contravariant location (the type of a formal parameter) is obviously significant for the body.

For covariant locations, (1) it matters a lot for the client whether the return type of a function which is being invoked is dynamic or another top type (and it doesn't matter for the body, so there's no conflict of interests), and (2) a similar logic applies to covariant locations which are more deeply nested:

void foo(void f(dynamic x)) {...}

main() {
  foo((x) => x.bar(42));
}

In this example, the single occurrence of dynamic is covariant, and it matters for the client because it becomes the type annotation of the formal parameter x of the function literal passed to foo in the body of main.

So my recommendation would be to keep flagging this kind of "implicit dynamic" for now, and then later on use variance based top type erasure to help developers route these effects only along the paths where they make sense.

@srawlins
Copy link
Member

Bump. We're playing with --no-implicit-dynamic, and hitting this in several places, e.g. Intl.plural:

  • Map<String, dynamic> examples (--no-implicit-dynamic complains when, e.g. 'widgets': 123} is passed as an argument)
  • List args (--no-implicit-dynamic complains when, e.g. [a, b] is passed as an argument) - for this one I would expect --no-implicit-dynamic to continue to complain until the signature is changed to List<dynamic>.

@stereotype441
Copy link
Member

Judging from the comments on Feb 4-5, it sounds like the language team doesn't have a consensus on what the expected behavior should be. @leafpetersen, @eernstg, @lrhn, can you make a recommendation on how we should proceed?

@leafpetersen
Copy link
Member

I think my take is that --no-implicit-dynamic isn't really what we want, and that what I wrote up in my "simple strict static checking" doc is closer to what we want.

@srawlins
Copy link
Member

srawlins commented Dec 28, 2018

That makes sense. I agree. We're trying to figure out of we can pursue using --no-implicit-dynamic until (simple) strict static checks can be written. This issue ("implicit dynamic" complaints where inference gives dynamic anyway) may leave --no-implicit-dynamic unhelpful, as it requires too much explicit dynamic.

It's an effort question then, re: --no-implicit-dynamic and strict static checks.

@leafpetersen
Copy link
Member

I think that the changes that you would probably want to make to --no-implicit-dynamic to avoid the issue that you are running into are more or less the equivalent of implementing my proposed --strict-inference from the simple static checking doc (not the initial doc, which requires more work).

@aadilmaan aadilmaan modified the milestones: Future, D25 Release Jun 4, 2019
@imod
Copy link

imod commented May 4, 2020

any plans to fix this?

@srawlins srawlins added the P3 A lower priority bug or feature request label Dec 2, 2020
@leecommamichael
Copy link

leecommamichael commented Apr 22, 2021

I've had the privilege of introducing a few JS devs to Dart, and this is such a sticking point. I wanted no-implicit-dynamic because there are many potholes at runtime without it. It hurts to explain what generics are to someone that doesn't think in typed-languages to begin with, let alone that generic-parameters have separate syntax for class-definitions and value-literals.

@andesappal
Copy link

Wow this is still open?

@andesappal
Copy link

andesappal commented Apr 29, 2022

'Missing type arguments for map literal'

image

What explicit type should I add, and how and where? Thank you!

@srawlins
Copy link
Member

Sorry this has fallen below the radar. The --no-implicit-dynamic flag is soft-deprecated, so this will not be fixed.

The replacement is an analysis_options.yaml option: strict-raw-types: true.

@andesappal
Copy link

Thank you!

@andesappal
Copy link

@srawlins the error persists with the analysis_options.yaml being

analyzer:
  strong-mode:
    implicit-casts: false
    implicit-dynamic: false
  errors:
    close_sinks: ignore
  language:
    strict-raw-types: true

linter:
  rules:
    - annotate_overrides
    - avoid_empty_else
    - avoid_function_literals_in_foreach_calls
    - avoid_init_to_null
    - avoid_null_checks_in_equality_operators
    - avoid_relative_lib_imports
    - avoid_renaming_method_parameters
    - avoid_return_types_on_setters
    - avoid_returning_null
    - avoid_types_as_parameter_names
    - avoid_unused_constructor_parameters
    - await_only_futures
    - camel_case_types
    - cancel_subscriptions
    - cascade_invocations
    - comment_references
    - constant_identifier_names
    - control_flow_in_finally
    - directives_ordering
    - empty_catches
    - empty_constructor_bodies
    - empty_statements
    - hash_and_equals
    - implementation_imports
    - invariant_booleans
    - iterable_contains_unrelated_type
    - library_names
    - library_prefixes
    - lines_longer_than_80_chars
    - list_remove_unrelated_type
    - no_adjacent_strings_in_list
    - no_duplicate_case_values
    - non_constant_identifier_names
    - null_closures
    - omit_local_variable_types
    - only_throw_errors
    - overridden_fields
    - package_api_docs
    - package_names
    - package_prefixed_library_names
    - prefer_adjacent_string_concatenation
    - prefer_collection_literals
    - prefer_conditional_assignment
    - prefer_const_constructors
    - prefer_contains
    - prefer_equal_for_default_values
    - prefer_final_fields
    - prefer_initializing_formals
    - prefer_interpolation_to_compose_strings
    - prefer_is_empty
    - prefer_is_not_empty
    - prefer_single_quotes
    - prefer_typing_uninitialized_variables
    # - public_member_api_docs
    - recursive_getters
    - slash_for_doc_comments
    - sort_constructors_first
    - test_types_in_equals
    - throw_in_finally
    - type_init_formals
    - unawaited_futures
    - unnecessary_brace_in_string_interps
    - unnecessary_const
    - unnecessary_getters_setters
    - unnecessary_lambdas
    - unnecessary_new
    - unnecessary_null_aware_assignments
    - unnecessary_statements
    - unnecessary_this
    - unrelated_type_equality_checks
    - use_rethrow_when_possible
    - valid_regexps

Are there any linter rules I should remove?

@srawlins
Copy link
Member

strict-raw-types: true is the replacement for implicit-dynamic: false, so I would remove the latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests