Analyzer: strict_raw_types difficult to comply with atomically

[srawlins]

Basically I'd like to split up the STRICT_RAW_TYPE code into a few more, to make it an easier pill to swallow. Teams can either make their code compliant incrementally, or opt out of individual checks they feel they are too onerous. Something like:

• STRICT_RAW_TYPE_LIST_LITERAL for var foo = [];
• STRICT_RAW_TYPE_MAP_LITERAL for var foo = {};
• STRICT_RAW_TYPE_SET_LITERAL
• STRICT_RAW_TYPE_NAME for foo is List, foo as List, List foo, Future main(), main(List args), perhaps
• STRICT_RAW_TYPE_CONSTRUCTOR for Future.value(), Set()

[srawlins]

cc @bwilkerson @leafpetersen @matanlurey

[matanlurey]

Sounds good to me as long as there is a single flag we can enable in the future

[leafpetersen]

The first three aren't raw type issues, they're implicit dynamic issues, right?

I'm fine with splitting things up, but I do wonder whether, if this is turning out to be such a bitter pill, we should reconsider and perhaps do something at least incrementally more sophisticated?

[srawlins]

Yeah I was wondering that as well, Leaf. @jmesserly committed a lot of tests with the mode, so you can see some examples:

var rawConstructorCall = /*info:STRICT_RAW_TYPE*/C();
var rawList = /*info:STRICT_RAW_TYPE*/[];
var upwardsInfersDynamic = /*info:STRICT_RAW_TYPE*/[42 as dynamic];
var rawSetOfSets = </*info:STRICT_RAW_TYPE*/Set>{};
var upwardsInfersDynamic = /*info:STRICT_RAW_TYPE*/{42 as dynamic};
var rawMap = /*info:STRICT_RAW_TYPE*/{};
dynamic d;
var upwardsInfersDynamic1 = /*info:STRICT_RAW_TYPE*/{d: 2};

Not sure why I didn't comment on this during code review; I think all the "strict" modes have just been swimming around in my head. I agree these should be strict-inference: true or implicit-dynamic: false reports. If that's the case, then I can just keep that 4th example (foo is List, foo as List, List foo, Future main(), main(List args), class MyList extends List {}) as STRICT_RAW_TYPE. WDYT, @jmesserly ?

[srawlins]

Oh, and my plan, @leafpetersen, is to bring up strict codes one by one for review by a few user teams. I personally enjoy complying with, e.g., strict-raw-types, but for very complex type hierarchies, I'd like team input. And I think @optionalTypeArgs will really help here. I think this will make for an incremental transition.

[jmesserly]

Yeah, everything except STRICT_RAW_TYPE_NAME in @srawlins' list above could be argued to fall under strict-inference instead. I'd be fine with that split.

It may still be worth separate error codes within strict-inference, so constructors/literals are separate from generic functions/methods? I've noticed that users seem to conceptualize constructor/literal inference as distinct from generic method inference (even though they're essentially the same inference algorithm). So we may wish to reflect that in the error UX.

---

I'm fine with splitting things up, but I do wonder whether, if this is turning out to be such a bitter pill, we should reconsider and perhaps do something at least incrementally more sophisticated?

This has been my worry. Conceptually there is nothing wrong with no-implicit-dynamic but it proved too much. strict-raw-types along with strict-inference will be an improvement, because they're smarter: they focus on semantics (we had to guess a type) rather than syntax (a type wasn't written).

If we find those are still too hard to roll out, we could focus more narrowly. Implicit dynamic is fine until we'd do something that Object wouldn't allow, such as dynamic dispatch. So we could try to implement a rule like that, if needed. (The easiest way to implement it would be to change generic-inference/instantiate-to-bounds to use Object rather than dynamic as their default top type. IDK if that would worry the language team? EDIT: of course, this would only apply when the appropriate strictness flag is enabled.)

Overall, checking for implicit downcasts and unintended dynamic dispatch should also cover all of the potential runtime problems related to dynamic. (Of course, "unintended dynamic dispatch" is very tricky to define.)

[srawlins]

I have confidence that we can land the (now) strict inference literals internally; not a hard sell. And I think a further split of STRICT_RAW_TYPE_NAME, as @jmesserly suggests, would help to pinpoint any spots where it might be less beneficial; but I think there could be a dozen :P e.g

1. STRICT_RAW_TYPE_IN_DECLARATION for List foo (locals, fields, top-level)
2. STRICT_RAW_TYPE_IN_PARAMETER for main(List args)
3. STRICT_RAW_TYPE_IN_RETURN for Future main() (or combine with above as something like STRICT_RAW_TYPE_IN_FUNCTION_SIGNATURE)
4. STRICT_RAW_TYPE_IN_IS for foo is List
5. STRICT_RAW_TYPE_IN_AS for foo as List (or perhaps combine with above; STRICT_RAW_TYPE_IN_TYPE_PROMOTION or STRICT_RAW_TYPE_IN_EXPRESSION)?
6. STRICT_RAW_TYPE_CONSTRUCTOR for Future.value(), Set()
7. STRICT_RAW_TYPE_IN_CLASS_DECLARATION for class MyList extends List {}
8. STRICT_RAW_TYPE_IN_TYPEDEF_DECLARATION for typedef AsyncCallback = Future Function()
9. STRICT_RAW_TYPE_IN_TYPE_ANNOTATION for List<List> foo, class MyList extends List<Map> {} (or just walk a bit further up the AST from the raw type to find its in, e.g. a variable declaration or parameter, etc.)

Personally, I'm up for this; I can ask the "strict" teams about each individual check for their opinion or experience; I can mail out targeted changes which eliminate violations of each individual code.

Regarding dynamic invocation; this would be a pretty sweet check, but I think orthogonal; strict-raw-types and strict-inference are primarily designed to catch unintentional casts.

[leafpetersen]

This seems right to me. For the examples above, I would think of them as follows:

var rawConstructorCall = /*info:STRICT_RAW_TYPE*/C();  // raw type
var rawList = /*info:STRICT_RAW_TYPE*/[]; // strict inference
var upwardsInfersDynamic = /*info:STRICT_RAW_TYPE*/[42 as dynamic]; // probably no implicit dynamic
var rawSetOfSets = </*info:STRICT_RAW_TYPE*/Set>{}; // raw type
var upwardsInfersDynamic = /*info:STRICT_RAW_TYPE*/{42 as dynamic}; // no implicit dynamic
var rawMap = /*info:STRICT_RAW_TYPE*/{}; // strict inference
dynamic d;
var upwardsInfersDynamic1 = /*info:STRICT_RAW_TYPE*/{d: 2}; // no implicit dynamic

Splitting out the additional raw types could be useful, not sure.

[jmesserly]

@leafpetersen -- just for curiosity's sake, what's the mental model for treating list/set/map literals as distinct from constructor invocations?

We don't have rest parameters, but if we did, these two seem equivalent:

class C<T> {
  C(List<T> ...args); // made up syntax for rest parameters.
}
f() {
  // with some variables: x, y, and z in scope
  var list = [x, y, z];
  var c = C(x, y, z);
}

It seems like they're all conceptually constructor invocations, and use the same inference algorithm. But maybe that's my bias towards semantics :). Syntactically "C" is an identifier referring to a class (similar to "List"), whereas "[]" does not refer to a class directly (i.e. it implies the List class, but does not name it explicitly).

[jmesserly]

Thinking about this more... in new C(), C refers to a constructor rather than a type. Whereas in the type annotation like C c;, C refers to a type. So maybe the constructor invocations belong under "strict-inference"?

[srawlins]

Oops, yes, 100% agree, @jmesserly ; I would mark those as strict-inference; I'll make this change.

[leafpetersen]

@leafpetersen -- just for curiosity's sake, what's the mental model for treating list/set/map literals as distinct from constructor invocations?

I'm not sure I am treating them differently, but maybe I am. My thinking is this:

class C<T> {
  C([T x]);
  C.named();
}

void test() {
  new C();
  new C.named();
}

I think I would definitely argue that the first constructor call is definitely a strict inference failure: inference has to guess. It is also arguably a strict raw type failure, since the way that inference guess is by treating it as a raw type.

The second constructor call feels a little different to me, but probably isn't. Inference still has to guess, but in this case it always has to guess (unless there's a downward context). So probably should still be a strict inference failure? And also could arguably a strict raw type failure.

So maybe the constructor invocations belong under "strict-inference"?

I think they should definitely fall under strict inference. If we allow people to specify strict raw types independently of strict inference, we could flag them there as well, but I think this is probably not worthwhile. I'd say make strict raw types strictly about types.