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

Strong mode `missing_return: error` does not warn missing return for lambda #28233

Closed
ScottPierce opened this issue Jan 3, 2017 · 35 comments

Comments

Projects
None yet
@ScottPierce
Copy link

commented Jan 3, 2017

Strong mode doesn't seem to be warning when a return type is missing in a lambda, when I've explicitly listed the option missing_return: error. It seems to work fine for normal class methods. I'm using IntelliJ IDE.

I'm using flutter and here is an example:

child: new PageableLazyList(
    itemCount: icons.length,
    itemBuilder: (BuildContext context, int start, int count) {
      List<Widget> widgets = [];

      for (num i = start; i < start + count; i++) {
        final icon = icons[i];
        widgets.add(
            new Container(
                key: new ObjectKey(icon),
                padding: const EdgeInsets.all(12.0),
                child: new Card(
                    child: new Center(
                        child: new Icon(icon, size: 128.0, color: color)
                    )
                )
            )
        );
      }

    })

The itemBuilder is defined via the following typedef:
typedef List<Widget> ItemListBuilder(BuildContext context, int start, int count);

Notice in the first piece of code that their is no return statement, even though a return type of List<Widget> is enforced by the typedef. I get no warning or error from the IDE.

I've enabled strong mode with a configuration I copied and modified from the flutter library:

analyzer:
  language:
    enableStrictCallChecks: true
    enableSuperMixins: true
  strong-mode: true
  errors:
    # allow overriding fields (if they use super, ideally...)
    strong_mode_invalid_field_override: ignore
    # allow type narrowing
    strong_mode_down_cast_composite: ignore
    # allow having TODOs in the code
    todo: ignore
    missing_return: error
  exclude:
    - 'bin/cache/**'
    - 'packages/flutter_tools/test/data/dart_dependencies_test/**'

linter:
  rules:
    # these rules are documented on and in the same order as
    # the Dart Lint rules page to make maintenance easier
    # http://dart-lang.github.io/linter/lints/

    # === error rules ===
    - avoid_empty_else
    # - comment_references # blocked on https://github.com/dart-lang/dartdoc/issues/1153
    - cancel_subscriptions
    # - close_sinks # https://github.com/flutter/flutter/issues/5789
    - control_flow_in_finally
    - empty_statements
    - hash_and_equals
    # - invariant_booleans # https://github.com/flutter/flutter/issues/5790
    # - iterable_contains_unrelated_type
    - list_remove_unrelated_type
    # - literal_only_boolean_expressions # https://github.com/flutter/flutter/issues/5791
    - test_types_in_equals
    - throw_in_finally
    - unrelated_type_equality_checks
    - valid_regexps

    # === style rules ===
    - always_declare_return_types
    - always_specify_types
    - annotate_overrides
    - avoid_as
    - avoid_init_to_null
    - avoid_return_types_on_setters
    - await_only_futures
    - camel_case_types
    # - constant_identifier_names # https://github.com/dart-lang/linter/issues/204
    - control_flow_in_finally
    - empty_constructor_bodies
    - implementation_imports
    - library_names
    - library_prefixes
    - non_constant_identifier_names
    - one_member_abstracts
    # - only_throw_errors # https://github.com/flutter/flutter/issues/5792
    # - overridden_fields
    - package_api_docs
    - package_prefixed_library_names
    - prefer_is_not_empty
    # - public_member_api_docs
    - slash_for_doc_comments
    - sort_constructors_first
    - sort_unnamed_constructors_first
    - super_goes_last
    # - type_annotate_public_apis # subset of always_specify_types
    - type_init_formals
    # - unawaited_futures # https://github.com/flutter/flutter/issues/5793
    - unnecessary_brace_in_string_interp
    - unnecessary_getters_setters

    # === pub rules ===
    - package_names
@anders-sandholm

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2017

@jmesserly

This comment has been minimized.

Copy link
Member

commented Jan 3, 2017

I believe missing_return is a linter feature. It is not part of Strong Mode.

@ScottPierce

This comment has been minimized.

Copy link
Author

commented Jan 4, 2017

I stole it from the example here: https://www.dartlang.org/guides/language/analysis-options#configuring-specific-rules-for-analysis

It's used in the second example for that section.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

The missing_return hint currently only checks functions that have an explicit return type. It might be reasonable to extend this to closures when they appear as arguments to other functions (and hence we can know that a return type has been declared), but that could also be fairly confusing. For example, given

void f(int g()) {}

void main() {
  f(() {
    print('Got here');
  });
}

we could figure out that the argument to f is expected to return a value and produce a hint. But a very similar piece of code:

void f(int g()) {}

void main() {
  var g = () {
    print('Got here');
  };
  f(g);
}

would not produce any hint because at the point at which we're analyzing the closure we wouldn't know that it is going to be used in a context where a return value is expected.

It's probably better to not partially address this case.

@bwilkerson bwilkerson closed this Jan 4, 2017

@ScottPierce

This comment has been minimized.

Copy link
Author

commented Jan 4, 2017

@bwilkerson I see your point. In the second example you gave, couldn't you analyze the lambda g, see that it has no return type, and then properly warn that g doesn't meet the contract as a parameter for function f?

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

Sure, because the closure is being assigned to a local variable and we could use control flow to see what the value of that variable was at the point where f is invoked.

But the point was that, for any amount of static analysis we could choose to perform, we could fairly easily construct an example for which that amount of static analysis won't allow us to detect this problem. In other words, it's impossible to statically guarantee that we will never miss a case where an explicit return should have been provided but wasn't.

@floitschG

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2017

However, there is a difference for the closure that is used inside f. It's actual dynamic type is ()->int. That is, because of inference, it actually knows that it has a return type that is different from Object or dynamic. In that case, not having a return is clearly a mistake.

In some sense your example just highlights the fact that expressions can't just be assigned to locals, since the inference provides different types in that case.

foo(List<int> x) {}

foo([]);  // is different from:

var x = [];
foo(x);
@bwilkerson

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

Good point. Inference changes some of the rules.

@scheglov This might also have an impact on some of the refactorings (such as extract local variable) because performing the refactoring will change some of the type information.

@ScottPierce

This comment has been minimized.

Copy link
Author

commented Jan 4, 2017

So I've found this to be a particularly painful issue where dart isn't warning me where I've simply forgotten to declare a return type. It's bitten me several times, each time requiring quite a bit of hunting as it's not apparent in the stack trace where the NPE is coming from.

Given how much dart code uses lambdas, it seems reasonable to expect some safety from strong mode regarding usage of them. In the cases where static analysis couldn't be used to guarantee a lambda signature isn't correct, couldn't you simply offer a warning?

Perhaps this is outside the scope of the missing_return, but given how much I'm using lambdas in Dart, it seems odd to me that I can't use lambdas safely.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

Sorry, I forgot to re-open the issue. Both your pain and Florian's point that inference will infer a return type for the closure are convincing arguments.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

Interestingly I just filed the same issue. I agree with Brian that in the presence of inference it makes sense to issue a hint based on the inferred types.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

See also #26554

@eseidelGoogle

This comment has been minimized.

Copy link

commented Feb 8, 2017

Just noting from #26554 (and its dupes) that we've seen at least two flutter users (other than the reporter of this bug, so really three!) get confused here. One posted a stack overflow which eventually we traced to a missing return this could/should have caught and a second filed a flutter/flutter bug. No clue if those were just two unlucky anecdotes, but that's more reports than we get about many issues. :)

@lrhn

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

For the

void f(int g()) {}

void main() {
  var g = () {
    print('Got here');
  };
  f(g);
}

example, I would expect a warning/error in the f(g) expression. The type of g is/should be ()->void, and that's not assignable to ()->int.

That is also a reason why inferring a return type of Null where it rightfully should be void, is problematic. As a return type, inferring Null is equivalent to turning off most checks, just like dynamic used to be, since Null is now assignable to any type.

So, what are the actual rules for inferring function expression return types?

  • Use the context type if one is available (downward inference),
  • otherwise, if it's a => function, use the static type of the body expression,
  • otherwise look at return statements in the body:
    • if none, infer void.
    • if only return; statements, infer void.
    • otherwise infer an upper bound of all the static types of the return statement expressions (which may be void if you return the result of a void expression).

?
(I think it currently infers Null instead of void, but that should be fixed)

Then, if the return type came from downward inference, check that the return statements match the inferred type (no return or return; -> return type should be void or dynamic, for return e;, the type of e should be assignable to the return type).
(Plus the normal complications for async[] and sync functions.)

@eseidel

This comment has been minimized.

Copy link

commented Feb 8, 2017

Looks like we had yet another flutter user confused by this last night (after my previous comment):
https://gitter.im/flutter/flutter?at=589ad6a71465c46a56388d4d
Maybe we have something wrong in one of our example which causes folks to keep hitting this.

@ScottPierce

This comment has been minimized.

Copy link
Author

commented Feb 8, 2017

@jmesserly jmesserly reopened this Aug 23, 2018

@jmesserly

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

reopening since this was reverted. Unassigning myself, but it should be possible for someone on Analyzer to re-land this by reverting the revert.

As far as I know, nothing was wrong with my fix, it's simply that many of our build systems promote hints into build errors(!), thus it is not practical to ever fix bugs in hints. I hope we can improve these processes. Perhaps tweaking my fix to use a different hint code would avoid some of the issues.

@jmesserly jmesserly removed their assignment Aug 23, 2018

@natebosch

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

I think flutter is ignoring this hint for now and fixing the cases it surfaces. AFAIK internal code has, or can be, cleaned up. Should we reland this?

@devoncarew

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

@Hixie has a PR that fixes the known issues and re-ables the hint - flutter/flutter#20844.

AFAIK internal code has, or can be, cleaned up.

@keertip or @srawlins would have a handle on the number of internal issues - we should address them (or special case the hint) before re-landing so we don't cause any hiccups in the roll.

As far as I know, nothing was wrong with my fix

The fix itself was fantastic - it finds additional, real problems in user code - but because of our roll processes, we do need some downstream prep.

@keertip

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

The internal code has not been cleaned. There were too many cases of new missing returns that it would have taken us weeks to get the code cleaned and the SDK released.

Not sure what is the best way to reland this, it would be great if we could find a way to get users to fix their code as they make changes.

@jmesserly

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Yeah totally agree, it doesn't seem worth it to spend days/weeks cleaning things up. (Hopefully with Dart 2 out the door, we aren't going to be doing massive cleanups like that anymore. :) )

We could use a different hint code for when a hint shows up because of the fix (e.g. instead of missing_return, use missing_return_inferred). Maybe that would make it easier to whitelist the new behavior without affecting existing behavior. Could also split it into more different hint codes too, if some of them are more noisy than others. Not sure if that would help.

@srawlins

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Changing the analyzer code wouldn't really help get it landed internally. Just as easy to whitelist "new" violations of missing_return.

@srawlins

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

I think it would only be a few days of cleanup, and I think it would be worth it.

@DanTup

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

Out of curiosity what are there issues showing up in internal code? Are they false-positives (code is fine as-is and the lint picks up too much) or real issues?

I saw one in Flutter that was triggered by calling exit() inside a function (throwToolExit) which I understand (I guess this is similar to exceptions, though they can be handled with @alwaysThrows?) but I'm guessing that's not common.

@srawlins

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

Almost all real issues. A few cases where @alwaysThrows fixes. Nothing that seemed like a bug in https://dart-review.googlesource.com/c/sdk/+/66340.

@escamoteur

This comment has been minimized.

Copy link

commented Sep 28, 2018

It took me just yesterday an hour to find a missing return after I converted a => lambda to a block lambda. See here:

image

  RxCommand<ImageStorageRequest, ImageLocation> uploadImageCommand;

Event adding the generic parameters to createAsync did not help.

@zoechi found it

@srawlins

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

We can re-land this. Not hard to do the internal clean-up.

@goderbauer

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

I’d love to see this one re-land.

dart-bot pushed a commit that referenced this issue May 22, 2019

Reland fix for #28233: add hint for missing returns to function expre…
…ssions

The bulk of this change is actually correcting missing returns in the Dart SDK.

Bug: #28233
Change-Id: I52bcbc6c6f4a129d3fc22003f4448a7c3d4487ad
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/100301
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
@stereotype441

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@srawlins can we close this bug now?

@stereotype441 stereotype441 referenced this issue May 23, 2019

Merged

Pass an async callback to testWidgets. #33260

8 of 9 tasks complete
@srawlins

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Apologies, missed this on my commit message. Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.