Remove comment-based syntax for generic methods

[munificent]

We currently support and in some places still use the transitional /*<T>*/ syntax for generic methods. Now that real syntax is supported, we should phase out and then eliminate the old syntax.

I can file tracking bugs for the sub-issues if people want, but, for now, here's the known subtasks:

• Add a deprecation notice when the old syntax is detected. (Do we need to do this?)
• Remove uses of the syntax in our core libraries.
• Remove uses of the syntax in other packages and applications we maintain.
• Remove support in analyzer for parsing the comments and treating them as annotations.

Currently, there are a few places where we still deliberately use the old syntax:

• The LineSplitter class behaves differently depending on if you enable strong mode or not.
• Some 'as' checks are marked as as Object /*=ConcreteClass*/ for performance reasons because implementations do not currently take advantage of strong mode.

We'll want to ensure those are addressed first before the syntax is removed.

[bwilkerson]

We should also remove uses in our own code before removing support from analyzer.

[munificent]

Yes! I didn't intend that to be an ordered list, but I can see how it looks like it. Re-ordered. :)

[srawlins]

That LineSplitter behavior you note, is there an issue for that?

No, I didn't file a tracking issue, and Florian hasn't either. I can when we get closer to working on it.

Is it an issue?

Yes, but @floitschG knows better than I do. I believe the issue is that the existing API is basically un-statically-typeable. It claims to be of a type that either it isn't, or that can't be expressed. I don't recall the details.

Changing it to something that makes sense in strong mode would be a breaking change, so instead, it uses /*<T>*/ syntax to have a different type in strong mode than it does in spec mode.

[anders-sandholm]

At least the "Add a deprecation notice when the old syntax is detected" part should be 1.23.
No?

[dgrove]

Is this actually happening for 1.23? I don't think there are open tasks for, say, the analyzer for 1.23.

[leafpetersen]

@floitschG how close are we to eliminating this from the core libraries?

@srawlins Is internal code in a good state with respect to this yet?

@kevmoo Do we have a plan for external packages?

[kevmoo]

@leafpetersen I'd LOVE if we had a milestone w/ a hint/lint that told folks if they have these anywhere and help folks migrate.

@bwilkerson @pq – possible?

[floitschG]

Will be removed when we don't support dart 1.x semantics anymore.

Edit:
There are some areas, where strong-mode code behaves differently.
Dart2js needs it to provide the same performance as without the comment syntax.

[bwilkerson]

It's quite possible to have a hint warning people that this functionality is about to go away, but I'm not sure whether that's the right UX. While it would be nice to be told that these issues exist and to provide help finding all of the places that need to be fixed, having several hundred to thousands of hints in the Dart Analysis view would be annoying.

@jwren @devoncarew Any ideas for a better UX? Maybe we could filter them by default (and explain how to see the rest).

[kevmoo]

Better UX: have a tool that auto-converts things. Even having it as a lint (opt-in) would be great for folks that want to make sure they've migrated everything. Otherwise we can just run grep – but that's a less-than-ideal solution for our external users

…

On Thu, Jun 15, 2017 at 9:10 PM, Brian Wilkerson ***@***.***> wrote: It's quite possible to have a hint warning people that this functionality is about to go away, but I'm not sure whether that's the right UX. While it would be nice to be told that these issues exist and to provide help finding all of the places that need to be fixed, having several hundred to thousands of hints in the Dart Analysis view would be annoying. @jwren <https://github.com/jwren> @devoncarew <https://github.com/devoncarew> Any ideas for a better UX? Maybe we could filter them by default (and explain how to see the rest). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28796 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCivM1R8TlA1O1-V5VTBL81rPWY6g5ks5sEYGrgaJpZM4MDiiG> .

[srawlins]

@leafpetersen internal code has many examples of using the example syntax, but it should be easy to clean.

I only see a few instances of using as dynamic /*=MyClass*/. The SDK definitely has more.

[devoncarew]

A hint makes sense - something like a warn on deprecated language features hint.

[srawlins]

This is blocked by #28773 (which I think is essentially the 2nd checkmark in this issue's description).

[srawlins]

This is also soft blocked by #31091; customers cannot prepare for real-only syntax by migrating methods named get to real-only syntax, until there is a dev release with #31091 fixed.

[rakudrama]

When I patched https://dart-review.googlesource.com/c/sdk/+/8705, I saw some significant regressions on dart2js and dart2js html benchmarks.

[srawlins]

Done?

[munificent]

Think so!