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

Stateless Widget template has warnings #3347

Closed
eseidel opened this issue May 16, 2021 · 16 comments
Closed

Stateless Widget template has warnings #3347

eseidel opened this issue May 16, 2021 · 16 comments
Labels
in flutter Relates to running Flutter apps is bug
Milestone

Comments

@eseidel
Copy link

eseidel commented May 16, 2021

Describe the bug
A clear and concise description of what the bug is.

If you use the stateless widget template (which I think comes from Dart-Code?) it now ends up with warnings as of the latest Flutter default template?

image

To Reproduce

  1. flutter create to start a new empty project.
  2. Type 'stls' and let it autocomplete to the stateless widget template.

Expected behavior
Should not have any analyzer warnings.

First it gives you a warning about missing "key" parameter in constructor:
"Use key in widget constructors."

image

Then, once you fix that, then it gives you a new warning:

image
"Prefer declaring const constructors on @immutable classes."

Versions (please complete the following information):

[√] Flutter (Channel master, 2.3.0-2.0.pre.237, on Microsoft Windows [Version 10.0.19041.928], locale en-US)
    • Flutter version 2.3.0-2.0.pre.237 at C:\Users\micro\flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision c71e8bacfd (76 minutes ago), 2021-05-16 17:44:01 -0400
    • Engine revision 0c9ffcc623
    • Dart version 2.14.0 (build 2.14.0-edge.a527411e5100a0a4f48c4009087a1b988aa784af)

[√] Visual Studio - develop for Windows (Visual Studio Community 2019 16.9.4)
    • Visual Studio at C:\Program Files (x86)\Microsoft Visual Studio\2019\Community
    • Visual Studio Community 2019 version 16.9.31205.134
    • Windows 10 SDK version 10.0.19041.0
@eseidel eseidel added the is bug label May 16, 2021
@DanTup DanTup added this to the v3.23.0 milestone May 17, 2021
@DanTup DanTup added the in flutter Relates to running Flutter apps label May 17, 2021
@DanTup
Copy link
Member

DanTup commented May 17, 2021

Thanks! I think the fix here is to add the following to both stful and stless snippets:

  const $1({Key? key}) : super(key: key);

This resolves the issue in a newly-created project. However, in an existing project that is not opted into null-safety, this will result in errors on the ?. If we remove the ? then we'll have errors in null-safe projects (no default value or required) - although if I understand correctly that might be what IntelliJ is doing (flutter/flutter-intellij#5405).

To fix this properly, I think the snippets should come from the server (dart-lang/sdk#37831) where it can tell whether null-safety is enabled. I think in the meantime the current lint warning may be better than producing hard errors for some users, though I'm interested in others thoughts (@eseidel @devoncarew).

@DanTup
Copy link
Member

DanTup commented May 17, 2021

@bwilkerson I'm thinking about how to move these to the server without breaking anything (or causing duplicates). I think a possible plan would be first to ship some change sin Dart-Code:

  • Dart-Code starts sending a custom capability in LSP initialisation saying it supports server-provided snippets
  • If server capabilities say server will provide snippets, Dart-Code disables its local snippets (initially, with no changes to the server this will not happen and everything continues as normal)

Once that has rolled out, the server can be updated so that if the client capabilities say server-snippets are supported:

  • the server capabilities say the server will provide snippets
  • a SnippetContributor is enabled in the completion manager

In the future, once we're sure the original Dart-Code changes have reached enough users, we could remove the client capability or change the default so that LSP clients do not need to explicitly opt-in to it with a custom flag (beyond advertising support for the snippet syntax).

In the server we'd have two advantages over having them in the client:

  1. Ability to tell if null-safety is enabled for the library a snippet is being inserted to, so it could conditionally add ?s where required
  2. Ability to insert snippets only where they are allowed (currently VS Code will show snippets that insert class definitions in places they're certainly not valid).

I'm not sure how non-LSP clients could safely enable the snippets without something additional in the protocol (or sniffing the client version) though. If enabled for clients that don't know about them, there could end up being dupes.

@eseidelGoogle
Copy link

FYI @goderbauer, I believe these come from the new flutter_lints package included in the create template. (Not good or bad, just connecting folks.)

@AlexV525
Copy link

Ability to tell if null-safety is enabled for the library a snippet is being inserted to, so it could conditionally add ?s where required

Does the IntelliJ plugin able to do this? Or it still needs some improvements?

@DanTup
Copy link
Member

DanTup commented May 19, 2021

Based on the changes at flutter/flutter-intellij#5405, I think IntelliJ probably has a similar dilemma. The merged templates do not include a ? but I think that will produce errors (that the variable has no default value and is not marked nullable). I think putting the ?s in would result in any existing projects that are not opted-in to null-safety getting warnings that ? is not valid.

@AlexV525
Copy link

Yes currently it does have that issue. So it would be great if the plugin can have the same ability to detect NNBD.

@bwilkerson
Copy link

I'm thinking about how to move these to the server without breaking anything (or causing duplicates).

Sorry for the delay, I somehow missed the email when you first referenced me.

I'm absolutely in favor of moving this functionality into the server.

However, I'm not convinced that code completion is the right place to surface this. If VS Code doesn't have any other way of providing this functionality then I suppose that's probably how we'll need to handle it for LSP clients, but I think that IntelliJ will need a separate API. (If that's all true then we can worry about IntelliJ separately.)

What about other LSP clients? Do we want to be able to enable this in other clients? In order to do so would we need to make it unconditional (I assume that many other clients won't be able to use a custom protocol. If that's the case, then perhaps we should use the server's version (or a custom capability flag) to signal that these snippets will be provided and not require the client to send a "supports server-provided snippets" capability.

I'm also not sure at this point whether/how to provide this in Cider.

Perhaps the next step would be to create an issue in the sdk repo to make it easier to involve other interested parties.

@eseidelGoogle
Copy link

Since NNDB is the future and this transition period is temporary, should we consider moving all these things to use nndb by default and maybe only then (separately?) consider if they should be configurable (via server or otherwise) to non-nndb? I'm not sure what the % of projects with null-safety enabled is (or even if we have that stat).

@goderbauer
Copy link

While we don't have a way to configure it, I would also be in favor of generating null-safe code. It is what we recommend for new projects and we also want to nudge people to migrate their existing projects.

@DanTup
Copy link
Member

DanTup commented May 19, 2021

@bwilkerson

However, I'm not convinced that code completion is the right place to surface this. If VS Code doesn't have any other way of providing this functionality then I suppose that's probably how we'll need to handle it for LSP clients, but I think that IntelliJ will need a separate API. (If that's all true then we can worry about IntelliJ separately.)

It is currently the only way - snippets are included through the completion APIs (there is a specific completion kind for them).

If that doesn't seem best for IntelliJ, having it separate in the server and combining them for LSP in its completion handler sounds reasonable to me.

What about other LSP clients? Do we want to be able to enable this in other clients? In order to do so would we need to make it unconditional

Yep, my plan was to invert the default soon after shipping a Dart-Code that uses the flag - although as long as the server provides a flag back to the client saying when it will include them, we can probably default to opt-out right away as long as we ship a Dart-Code that can read that flag first (and allow a little time to assume the significant majority are updated). I'm less keen on using versions though - it's different to how other LSP things work and may make it harder for other clients to control this if they wanted.

Perhaps a user setting would actually work better - that way the user can toggle it themselves and we can default to on if not supplied. That avoids polluting capabilities and also allows changing it without restarting the editor.

Perhaps the next step would be to create an issue in the sdk repo to make it easier to involve other interested parties.

Makes sense, will do!

In the meantime, I think I agree with @eseidel and @goderbauer that updating to the null-safe versions shipped in the editor probably makes sense. It'll cause errors for people that aren't migrated, but I think it's clear why and the fix is relatively easy. Even if the analyzer gained support today, it would be a while before it reached the stable releases.

I'm still keen to move to the server though - one of my biggest complaints is that in VS Code we don't have enough context to provide them based on the location, so they show up in bad places (and given we have snippets with names like test I find myself triggering them when I'm just testing other things quite frequently!).

@DanTup
Copy link
Member

DanTup commented May 19, 2021

There was an existing SDK issue about this, so I added some notes and references back here -> dart-lang/sdk#37831

@bwilkerson
Copy link

Perhaps a user setting would actually work better ...

I don't see support for user settings in LSP, so I assume that this approach would require changes in every client. I'd rather have the support be provided via code completion by default than to make it hard for other clients to provide this support.

@DanTup
Copy link
Member

DanTup commented May 19, 2021

@bwilkerson sorry, it's called configuration in LSP not settings:

https://microsoft.github.io/language-server-protocol/specifications/specification-current/#workspace_configuration

It's not guaranteed that all clients support it, but I suspect it's very common. It's already being used it for a few things:

https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/lsp/client_configuration.dart

@DanTup
Copy link
Member

DanTup commented May 19, 2021

I'd rather have the support be provided via code completion by default than to make it hard for other clients to provide this support.

To clarify - I agree it should be provided by code completion and (at least ultimately) defaulted to on for LSP. It should not require any work in the LSP clients, they should just get it for free by implementing the standard LSP messages. But to avoid disruption for Dart-Code, there will need to be some way for:

  • Dart-Code to indicate to the server that it knows about server-provided snippets (so the server won't send them to a version of Dart-Code that can't hide its own)
  • Dart-Code to know that the server will provide those snippets (to avoid hiding its own and then not getting any from the server)

If we want to avoid a capability (in each direction) I think we could use a configuration setting. We could add that to Dart-Code now and set to false and ship that (it will do nothing, because nobody is reading it). Then, when we implement it in the server it can be toggled by that setting (the default can be true in the server so it's opt-out, and the shipped versions of Dart-Code would be opting-out with their default). Then Dart-Code can change its default to true/undefined in a future update (after the analyzer changes reach a stable release) and/or gated on the SDK version number (which is already fairly common.

@DanTup DanTup closed this as completed in 2d2f7e2 May 19, 2021
@bwilkerson
Copy link

I don't have a preference between those two options; I'm happy to let you decide.

@DanTup
Copy link
Member

DanTup commented May 19, 2021

I'd lean towards the setting - the capability wouldn't make as much sense if/when the default is true (sending a capability to disable something doesn't feel like a capability), and should another editor (or user) decide they did want to disable them, it's more complicated with a custom capability than just a basic user setting.

For the original issue here, I've updated the shipped snippets to assume null-safety for the next release. The change adds a const constructor that has a Key key argument, and also adds late to the AnimationController in the "StatefulWidget with an AnimationController" snippet (to match the docs on AnimationController).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in flutter Relates to running Flutter apps is bug
Projects
None yet
Development

No branches or pull requests

6 participants