[Feature Request] Add a way to provide a custom error message for using a missing parameter #43942

gspencergoog · 2020-10-27T21:33:51Z

In flutter/flutter#68736 (and bunch of other PRs), we're removing a parameter called nullOk that used to let the function know if the caller cared that the result was found or not (and hence whether it would return null or throw).

We were going to deprecate it, but decided that the failure mode of asserting that the value was false was worse than having a breaking change where we just remove the argument and change the return type to non-nullable, since the assert only happens at runtime, and the breaking change will catch the problem at compile time.

However, when we do that, we'd like to be able to remove the parameter, but have the compiler fail when someone tries to use it with a helpful message that says "use this instead", much like an @Deprecated annotation would.

I'm not sure what form that would take in the code, but I'm envisioning something like:

MediaQueryData of(
  BuildContext context, {
  @Obsolete(
    bool nullOk,
	'The nullOk parameter has been removed. '
    'Instead of setting the nullOk parameter, consider whether you want to use "of" or "maybeOf" ' 
    'instead.'),
}) { /*...*/ }

cc/ @leafpetersen @Hixie

The text was updated successfully, but these errors were encountered:

gspencergoog · 2020-10-27T21:40:04Z

I would think the error message would look like:

Error: No named parameter with the name 'nullOk'.
Obsolete: The nullOk parameter has been removed. Instead of setting the nullOk parameter, consider whether you want to use "of" or "maybeOf" instead.
  MediaQuery.of(context, nullOk: true);

leafpetersen · 2020-10-28T05:14:41Z

cc @devoncarew @bwilkerson This feels like maybe something that could be/should be done in the tooling, if we do anything? There could be a general language feature here, but if feels more like something that we provide on the side. cc @lrhn @natebosch @mit-mit is this something we would potentially use in the Dart sdk or Dart owned packages?

@devoncarew @bwilkerson I wonder whether this might not just fit naturally into something like the refactoring tool you've been working on?

Hixie · 2020-10-28T06:29:31Z

I definitely wouldn't want to hard-code Flutter framework deprecation messages in the VM. That seems like a layering violation. (I'm not a great fan of how we do the widget line number feature for the same reason.) In principle the framework is not special. Anyone should be able to create a framework like ours and it should work just as well.

bwilkerson · 2020-10-28T15:03:11Z

I wonder whether this might not just fit naturally into something like the refactoring tool you've been working on?

The data file used by data-driven fixes will have enough information in it to allow the error reporter to detect that the parameter used to be declared, so that's a start. The file doesn't currently have any way to express the 'obsolete' message, but that could be added. It would also require some refactoring. Currently the data-driven fixes support is in the analysis_server package and the error reporting is in the analyzer package, so we'd need to move it in order to make use of it in the analyzer.

The other possibility that was mentioned is to provide some way to add metadata to the function that could be used by the analyzer. I think the annotation would need to be on the function, rather than in the parameter list (where there might not be a parameter to associate it with), but it could potentially look something like

@RemovedNamedParameter(
  'nullOk',
  'The nullOk parameter has been removed. '
  "Instead of setting the 'nullOk' parameter, consider whether you want to use 'of' or 'maybeOf' "
  'instead.')
MediaQueryData of(BuildContext context) { /*...*/ }

I assume that the Flutter team would want to have some kind of policy governing how long such a notice lived in the code base. There might also be implications for dartdoc, but I don't know whether it displays all annotations or only those that it cares about.

Hixie · 2020-10-28T17:44:00Z

cc @Piinks who is working on related policies.

gspencergoog · 2020-10-28T18:12:47Z

As I showed in the description, I would also like the annotation to include the type of the removed parameter: it could then also give hints when the type changes and someone supplies an incompatible old type of the parameter, not just when it is removed.

bwilkerson · 2020-10-28T18:33:22Z

I would also like the annotation to include the type of the removed parameter: it could then also give hints when the type changes and someone supplies an incompatible old type of the parameter, not just when it is removed.

I suspect that I don't understand what you're asking for. If you change the type of a parameter and there is code that's no longer passing in the correct type, then there's already an error with a message like

The argument type 'String' can't be assigned to the parameter type 'int'.

Were you thinking of something different, or did I completely misunderstand?

gspencergoog · 2020-10-28T18:41:52Z

I was just thinking that in addition to "The argument type 'String' can't be assigned to the parameter type 'int'.", we could give a hint as to what is an appropriate replacement, and how to migrate.

The argument type 'String' can't be assigned to the parameter type 'int'.
Migration hint: The "value" String parameter has been changed an integer value index. You can find a migration guide for this change at https://flutter.dev/docs/release/breaking-changes/value-string-to-int
	foo.bar(value: 'old string');

gspencergoog · 2020-10-28T18:44:01Z

It would only apply if there was an annotation on the API that included the old type, and the caller was trying to call it with the old type. Calling the example above with an enum, for instance, wouldn't trigger the hint.

bwilkerson · 2020-10-28T18:55:35Z

Ok, thanks. I'm not sure how often that case will arise, but I think it would be sufficient to have a Type valued parameter for the constructor if we want to support it.

lrhn · 2020-10-29T09:48:54Z

The annotation should be on the function (it can't go in the {...} parameter block because that requires there to be a parameter, it can't just contain an annotation).

Maybe something general like specifying a specific name parameter have been removed:

@BreakingChange.namedParameterRemoved(#nullOk, since: "2.5", message: """
Instead of setting the 'nullOk' parameter, consider whether you want to use 'of' or 'maybeOf' instead
""")
void theMethod(...) ...

or specifying the previous entire method signature if the change is more than a single named parameter.

@BreakingChange.signature<int Function(int, int, {bool nullOk})>(since: "2.5", message: """
Instead of setting the 'nullOk' parameter, consider whether you want to use 'of' or 'maybeOf' instead
"""
void theMethod(...)...

(at least if we had generic constructors, until then we'll probably need BreakingSignatureChange<...>, and we'd want to allow generic function types as type arguments).

We could also have @BreakingChange.removed{{Static,}{Method,Getter,Setter,Variable},Constructor} on classes and @BreakingChange.removed{Function,Getter,Setter,Variable} on libraries.

And we could also report @BreakingChange.typeChanged<signature> or @BreakingChange.{return,parameter}TypeChange for situations where all the parameters are still there, but some types have changed.

It is something that can be useful whenever a package makes breaking changes.
This would allow the analyzer to report a special message when you're doing something wrong which would have been correct earlier, but not if you're just being generally wrong.
It might even be possible to automatically generate the annotations (sans "message") if a tool could see both versions of the package.

For packages, breaking changes are usually heralded by a major version increment, but even in a major version bump, most of a package is usually left unchanged. This could help users figure out what has changed (and you can even generate a "change report" from the code and the annotations, without having to look at the previous version, at least if the annotations are complete).

For SDK changes, it requires the annotations to be in the Dart SDK.
That's not unreasonable, it is somewhat related to deprecated and Since which exists and are used in the SDK.
The big issue is scope creep. How many changes to we want to be able to report, and at which granularity.
Dart SDK libraries do do breaking changes occasionally, even outside of major versions (because we can't wait for a major version increment for important changes). We handle additions using @Since, but have nothing for removals (or rather, we are supposed to @deprecate for a good while before removing, which is intended to make sure noone gets surprised by the change.
So, not sure if it's worth having this in the SDK. That makes the annotation itself less flexible than if it's in package:meta, but it would allow us to use it for the SDK. (We could also just hardwire SDK changes into the analyzer, as if it was annotated, but without actually being so. Third party packages don't have that option.).

The Flutter changes is in a package, so they can depend on annotations in package:meta.

bwilkerson · 2020-10-29T15:12:11Z

... until then we'll probably need BreakingSignatureChange<...> ...

Unfortunately, we can't currently use type arguments in an annotation, which is why I proposed a Type valued parameter. (We might want to consider allowing that. They are allowed in other constant expressions (as long as the type argument can be known at compile time) and the restriction has never made much sense to me.)

We could also have @BreakingChange.removed{{Static,}{Method,Getter,Setter,Variable},Constructor} on classes ...

I'll just note that the data file for the data-driven fixes work actually has even more information than that, allowing analyzer to know things like when a declaration was renamed or completely removed, when parameters were added, when type parameters were changed, etc. It also contains enough information to allow the analysis server to fix the users code in many cases. I'm not convinced that we want to re-invent support that we are already implementing.

The reason we chose to put this information into a separate file is because of the case of removing an API. In that case there isn't anywhere to hang an annotation that could provide analyzer with the information it needs.

lrhn · 2020-10-29T15:19:06Z

I completely forgot that you can't use type arguments in annotations. We should remove that restriction as long as it's syntactically simple (not making parsing much harder), and I think it is that simple (there is no need for @id< to ever mean anything else than a type parameter to a const constructor).
The alternative is worse: Either encoding types as (most likely) strings, or using generic classes as arguments (type: TypeHelper<the type arguement>()) which is just syntactic overhead with no value.

Having the information on the side is a good thing, it means that you can (basically) know about all versions of the code.
The only advantage of the annotation would then be to add a custom message to show to users when they use the older APIs.

srawlins · 2024-08-02T14:20:17Z

(Note: we added generic metadata, so those ideas are possible.)

gspencergoog mentioned this issue Oct 27, 2020

Remove nullOk in MediaQuery.of flutter/flutter#68736

Merged

2 tasks

gspencergoog changed the title ~~[Feature Request] Add a way to provide a custom message for using a missing parameter~~ [Feature Request] Add a way to provide a custom error message for using a missing parameter Oct 27, 2020

leafpetersen added the legacy-area-analyzer Use area-devexp instead. label Oct 28, 2020

ductranit mentioned this issue Oct 29, 2020

Replace nullOk by maybeOf fluttercandies/extended_image#250

Closed

ebot64 mentioned this issue Oct 29, 2020

Error: No named parameter with the name 'nullOk'. nullOk: nullOk, aloisdeniel/flutter_device_preview#74

Closed

srawlins added the P3 A lower priority bug or feature request label Feb 3, 2021

srawlins added the type-enhancement A request for a change that isn't a bug label Mar 13, 2024

bwilkerson added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Feature Request] Add a way to provide a custom error message for using a missing parameter #43942

[Feature Request] Add a way to provide a custom error message for using a missing parameter #43942

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Feature Request] Add a way to provide a custom error message for using a missing parameter #43942

[Feature Request] Add a way to provide a custom error message for using a missing parameter #43942

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!