Skip to content

Never use as. #57241

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
pq opened this issue Oct 30, 2015 · 14 comments
Closed

Never use as. #57241

pq opened this issue Oct 30, 2015 · 14 comments
Assignees
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-lint-request type-enhancement A request for a change that isn't a bug

Comments

@pq
Copy link
Member

pq commented Oct 30, 2015

As per #57238.

@pq pq added type-enhancement A request for a change that isn't a bug linter-lint-request labels Oct 30, 2015
@pq
Copy link
Member Author

pq commented Nov 6, 2015

First cut at some descriptive text.

From the flutter style guide:

AVOID using as.

If you know the type is correct, use an assertion or assign to a more narrowly-typed variable (this avoids the type check in release mode; as is not compiled out in release mode). If you don't know whether the type is correct, check using is (this avoids the exception that as raises).

BAD:

try {
   (pm as Person).firstName = 'Seth';
} on CastError { }

GOOD:

Person person = pm;
person.firstName = 'Seth';

@Hixie
Copy link
Contributor

Hixie commented Nov 6, 2015

The "GOOD" for that "BAD" would be more like:

Person person = pm;
person.firstName = 'Seth';

The "BAD" for that "GOOD" would be more like:

try {
   (pm as Person).firstName = 'Seth';
} on CastError { }

@pq
Copy link
Member Author

pq commented Nov 6, 2015

Updated. Thanks!

@pq pq self-assigned this Nov 6, 2015
@zoechi
Copy link
Contributor

zoechi commented Nov 17, 2015

This seems to conflict with strong mode
I changed

options.include = JSON.decode(include) as Map<String, List<String>>; // warning: avoid as

to

Map<String, List<String>> json = JSON.decode(include); // error: will need runtime check 
options.include = json;

just to get the warning that it will need a runtime check.
@Hixie don't you use strong-mode? It seems to comply with your approach to get proper warnings for all type violations?
Or did I miss some way to work around one of these warnings?

@Hixie
Copy link
Contributor

Hixie commented Nov 17, 2015

We filter out that strong mode warning.

We don't actually use strong mode itself (i.e. DDC and its dart-to-js conversion). We do turn on the strong mode warnings in the analyzer, but only because they happen to catch some things we want to catch. They also catch things we don't want, and we have a script that filters those out:

https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/commands/analyze.dart

Ideally we'd have all the warnings we could want with strong mode off. It's not our goal to influence the DDC work, which has a very different purpose than what we're doing.

@zoechi
Copy link
Contributor

zoechi commented Nov 17, 2015

@Hixie thanks a lot for explaining your take on strong mode.

@stt106
Copy link

stt106 commented Nov 27, 2018

if the advice is "never use as", then maybe "as" should be removed from the language?

@zoechi
Copy link
Contributor

zoechi commented Nov 27, 2018

@stt106 that was for Dart 1. This is quite a different story in Dart 2.

@stt106
Copy link

stt106 commented Nov 27, 2018

@zoechi Then why in https://raw.githubusercontent.com/flutter/flutter/master/analysis_options.yaml it still says "avoid using as" ?

@zoechi
Copy link
Contributor

zoechi commented Nov 27, 2018

@stt106 #34582 not exactly sure how to interpret the last comment though.

@Hixie
Copy link
Contributor

Hixie commented Nov 27, 2018

We (Flutter) haven't removed that lint yet because we want to remove the lint at the same time as we update all the code to use as instead of implicit downcasts. It'd be even worse for the codebase to be inconsistent.

@truongsinh
Copy link

in the example
GOOD:

if (pm is Person)
  pm.firstName = 'Seth';

BAD:

try {
   (pm as Person).firstName = 'Seth';
} on CastError { }

I would rather prefer
BEST:

if (!(pm is Person)) {
  // throw or early return
}
pm.firstName = 'Seth';

Should I create a new issue asking for this enhancement, as currently linter is not able to understand this?

@Hixie
Copy link
Contributor

Hixie commented Mar 25, 2019

Try

if (pm is! Person)
  throw Exception('...');
pm.firstName = 'Seth';

...but note that that is a different semantic than the GOOD vs BAD case in the documentation. The point of the documentation's example is that a non-matching (and non-null) value will not result in an exception, it'll just be ignored. (In the case of the value being null, it'll throw in the GOOD case and not in the BAD case.)

In general, all three of these techniques aren't very good since they introduce knowledge of the class hierarchy outside of the interface defined by the class hierarchy, which breaks polymorphism.

@truongsinh
Copy link

thanks, I didn't know about is!, however, I don't think it works as expected:

void main() {
  dynamic a;
  List<dynamic> b;
  if (a is! List) {
    b = a; // expected, invalid_assignment
  } else {
    b = a; // UNEXPECTED; observed: invalid_assignment, expected no error
  }
  if (a is List) {
    b = a; // good, no error
  } else {
    b = a; // expected, invalid_assignment
  }
}

Btw, I think what i'm trying to achieve is similar to https://docs.swift.org/swift-book/ReferenceManual/Statements.html#grammar_guard-statement , which might be out of this issue's scope, or even linter's scope, but more into the language spec itself.

@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-lint-request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants