Analyzer complains about error in comment

[zoechi]

class EventBus {
  EventData fire(EventType /*<T>*/ eventType, /*<T>*/ EventData data) {
    ...
  }
}

Warning:(35, 31) The name 'T' is not a type and cannot be used as a parameterized type

I saw some code reviews where Dart team members added such comments for future generic method support, but I didn't expect this to have any effect yet.

I copied the code above from some other project and didn't bother to remove the comments and now get warnings.
Is this related to strong mode or default analyzer?

[zoechi]

Dart VM version: 1.14.0-edge.a689bf41805c251b5bae646e0eb82f832d1e309f (Wed Nov 18 17:36:06 2015) on "linux_x64"

[bwilkerson]

@zoechi This should only show up in strong mode, but it seems wrong to produce an error for code that is perfectly valid. Are you running with strong mode enabled?

@jmesserly Should we change this feature so that if the comments don't parse correctly we just silently ignore them? Or perhaps the errors should be behind a separate flag, especially given that this is a short-term work-around until we can get the generic method DEP approved.

[zoechi]

Yup, I have strong-mode enabled.

I started working on porting a Polymer 0.16 project to 1.0.0-rc.x and got a warning I had no idea where it came from.
Now with strong-mode, linter included in analyzer, experimental features, it's really hard to figure out what produces the hint/error/warning.

I'm fine with comments that have a meaning, they also can produce warnings. I like to get as much tooling support as possible.
I just really would love to get proper information what produced the warning. See also #57248
I think each warning/hint/info should contain an exact key that allows to look up where exactly it comes from like

Warning:(35, 31) The name 'T' is not a type and cannot be used as a parameterized type [SM123]

for strong mode error code 123 and a const someError = 123; somewhere in the GitHub repo containing the strong mode plugin source.

The linter seems to at least add [lint] somewhere (see also #57248), but no exact key.

The messages are sometimes really hard to tell apart. An exact key would make this much easier.

[bwilkerson]

I agree. We need error codes to make it easier (possible?) to find documentation for them.

I'm not sure why it matters where the error comes from, though. As long as the documentation said something like:

This warning is produced only when strong mode is enabled.

My concern is more about false positives, but perhaps that not a big concern, given that you're trying to document that you'd be using generic method syntax if it were available.

By the way, I think the following will let strong mode do what you want:

EventData fire/*<T>*/(EventType /*<T>*/ eventType, EventData /*=T*/ data) {
  ...
}

Note, however, that support for this form is short-term and will be removed without notice.

[jmesserly]

@jmesserly Should we change this feature so that if the comments don't parse correctly we just silently ignore them? Or perhaps the errors should be behind a separate flag, especially given that this is a short-term work-around until we can get the generic method DEP approved.

The problem here is it does parse correctly. It parses to EventType<T>, which is then an error because T is not a type.

I don't see how we can hide this message in the current design for the comments.

My original preference was to have it work quite differently. I wanted to support syntax like:

/*T*/ foo/*<T>*/(/*T*/ t);

Then, the comments would be totally ignored in the scanner/parser. They would only kick in during name binding, if and only if the type name matches one that is declared in a comment.

I believe that would give the desired error suppression behavior, as well as allow cleaner syntax (/*T*/ instead of /*=T*/).

[bwilkerson]

Would it support adding type parameters, such as (List /*<T>*/ list)?

[jmesserly]

@zoechi -- for better or worse:

• we need generic methods to make strong mode work,
• we have to encode generic methods in comments.

As a consequence, comments will become meaningful to --strong mode, at least in some cases.

That said, we can fix this case, but it does require completely reimplementing at what point we find the comments. (all of the scanner/parser code needs to be removed, instead we look for them later)

[jmesserly]

And I'm happy to fix this, thanks @bwilkerson for sending it my way! Let me know if the approach sounds right (revert scanner/parser changes, instead look for the comments later on, IFF we are in a generic function/method only -- which the example in this bug would not be, so we shouldn't even look for them)

[jmesserly]

Addendum: and thanks @zoechi for trying --strong and catching this! This kind of usability feedback is incredibly valuable.

[jmesserly]

Would it support adding type parameters, such as (List /*<T>*/ list)?

yup, I think so. I'd have to look in depth because it's a complete reimplementation. But basically we would have a notion that we have a type parameter T in scope, so we'd look for /*<T>*/ comment in visitTypeName (of TypeResolverVisitor?) ... we could delay parsing until that point. Unless both parsing and name resolution succeeds, ignore it.

[jmesserly]

(essentially, about every place in parser where we currently parse generic comments, we'd instead leave them unparsed and move the "search for a comment" to resolver.dart ... that lets us confirm that the syntax as well as the name resolution works before we accept it.)

[bwilkerson]

... look for the comments later on, IFF we are in a generic function/method ...

How would we determine that we're in a generic function?

More importantly, why couldn't we know that we're in a generic function while we're parsing?

Let me know if the approach sounds right ...

It sounds like overkill to me, but I'm probably missing something. Hence the questions.

[zoechi]

@bwilkerson

I'm not sure why it matters where the error comes from, though. As long as the documentation said something like:

There are ways to disable certain checks (linter, single lint rules, strong mode,..., hopefully more to come) but that only works if I know what needs to be disabled.

Just saw https://github.com/dart-lang/dart_enhancement_proposals/blob/master/Meetings/2015-10-28%20DEP%20Committee%20Meeting.md#suppressing-warnings
where this is also covered.

[bwilkerson]

There are ways to disable certain checks ... but that only works if I know what needs to be disabled.

Good point. I think that information ought to also be in the error's description.

[zoechi]

@jmesserly the error itself doesn't worry me at all and working around is easy, only that it's hard to figure out what creates the warning is an issue. At first I thought it's just another analyzer bug I run into.
Just because I saw a code review earlier today where such comments where added I got suspicious that this might be a new feature.

[jmesserly]

Yeah, the thing to keep in mind, it's the same error you'd get from:

class EventBus {
  EventData fire(EventType<T> eventType, /*<T>*/ EventData data) {
    ...
  }
}

So we can't suggest --strong-mode as the cause (or at least, I don't see an easy way to do that). Strong mode choose to parse the comment, but the name resolution error came later, by that time we can't tell that it was a comment (vs normal syntax).

@bwilkerson:

How would we determine that we're in a generic function?

type parameters would be in scope. (When we visit MethodDeclaration for example, we'd look for a type parameter comment.)

More importantly, why couldn't we know that we're in a generic function while we're parsing?

Disclaimer: you're way more familiar with the parser and its logical invariants than I am. So this could reflect my own misconceptions about the code structure. :)

So if I imagine implementing this in the parser: we don't know we're in a generic method at the point of parsing the return type, unless we want to try and implement more lookahead. That's the first issue I'd likely hit. Due to Dart's grammar, there's a lot of different paths that can parse types and functions and methods (in fact, I missed at least 1 case for top-level function return type.)

In general the parser doesn't have much semantic knowledge about the code, or state it tries to push/pop as it recurses into parsing subtrees. It's easy to do after the parser because we have the AST at that point, so the structure is known. (And we can also use the set of names in scope, if we'd like to.)

That said, it's certainly possible to track more state in the parser, if that's the route we'd like to go. I'd have to ensure that we know we're in a generic function before parsing the parameters & body. (Perhaps return type can be fixed up after the fact), and carefully push/pop that state. Maybe it's not too bad. I can certainly take a look.

Here's a question though. Say I have:

class EventBus {
  EventData fire/*<T>*/(EventType /*<T>*/ eventType, /*<T>*/ EventData data) {
    Map<String, dynamic/*=S*/> map ...;
  }
}

Do we want to parse that /*=S*/ as a type, because we're inside a generic function? That can lead to exactly the same error message that started this issue. To get rid of it, we'd have to know only "T" is in scope, which feels a lot like name resolution to me, which feels like something parser shouldn't be doing.

[jmesserly]

And it's very minor, but I've gotten some feedback about the /*=T*/ being slightly non-intuitive. So I'd like to support /*T*/, but that really needs name resolution to work, otherwise any single word comment would look like a type.

[leafpetersen]

I'm ok with changing this to only recognize the comments when we're in a generic method, but this does make life a bit more exciting for people using these annotations. If you make a typo in your generic parameter list (or put it in the wrong place), you won't get any error messages - your type annotations will just get silently ignored in favor of dynamic. I think I would lean towards at least making John's EventBus example an error. You've pretty clearly opted into using the generic method syntax there, so I think it's perfectly reasonable to emit an error on the ill-formed type parameter to Map.

[bwilkerson]

@jmesserly You're right, we'd have to postpone parsing the return type until after we've seen whether there are type parameters. In which case, it might well be easier to wait until after parsing. I don't have any special knowledge that you lack; I'm happy to trust your opinion.

[jmesserly]

added strong mode tag. I'm going to take a look at this relatively soon -- at the moment trying to finish up a few other issues related to generic methods.

[jmesserly]

(Bumping this to default priority-medium, since this only happens when strong-mode is on & does indicate a legitimate error is usage of strong mode's generic methods. At the moment I think we have some genuine correctness/stability issues, which I'm putting in the pri-high bucket. But it's definitely important to improve the UX of this error message somehow -- probably by changing when the generic method parsing kicks in.)

[munificent]

We spent some time talking about this. There are ways we could report avoid treating spurious comments like magic generic method comments when they actually aren't, but it would involve moving a lot of code around in the analyzer.

Having a great user experience definitely matters, but we're hoping to get real non-comment generic method syntax in soon. Given that, we don't want to burn too much time on polishing the UX around the comments since we hope to tear that code out anyway. I'm going to close this out.

Thank you for filing the issue!

[zoechi]

Fine for me of course. When I encountered the issue it was an interesting discovery that there is ongoing work on generic methods. Working around is easy enough once you know what causes the error. Thanks for the great work!