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

useless The argument type 'String?' can't be assigned to the parameter type 'String'. #59820

Open
stephane-archer opened this issue Dec 30, 2024 · 33 comments
Labels
analyzer-ux analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-question A question about expected behavior or functionality

Comments

@stephane-archer
Copy link

class _SmartExposureAnalysisWidget extends StatelessWidget {
  const _SmartExposureAnalysisWidget({
    required this.pathImgSelectedLut,
    required this.imagePath,
    required this.previewGeneratorChangeNotifier,
  });

  final String? pathImgSelectedLut;
  final String? imagePath;
  final PreviewGeneratorChangeNotifier previewGeneratorChangeNotifier;

  @override
  Widget build(BuildContext context) {
    if (imagePath == null) {
      return const SizedBox.shrink();
    }
    if (pathImgSelectedLut == null) {
      return ExposureAnalysisWidget(
          imagePath, previewGeneratorChangeNotifier);
    }
    return ExposureAnalysisWidget(
        pathImgSelectedLut, previewGeneratorChangeNotifier);
  }
}

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

Because imagePath is final and is checked for null in the build function, I don't see why I should have a ! or a local variable to avoid this error message.

Am I missing something?

@dart-github-bot
Copy link
Collaborator

Summary: Null-safe String? variables are passed to a function expecting non-nullable String arguments. The code already handles null checks; the error is misleading.

@dart-github-bot dart-github-bot added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-question A question about expected behavior or functionality labels Dec 30, 2024
@stephane-archer
Copy link
Author

here is the fixed code without using !, it seems really unecessary

class _SmartExposureAnalysisWidget extends StatelessWidget {
  const _SmartExposureAnalysisWidget({
    required String? pathImgSelectedLut,
    required String? imagePath,
    required this.previewGeneratorChangeNotifier,
  })  : _pathImgSelectedLut = pathImgSelectedLut,
        _imagePath = imagePath;

  final String? _pathImgSelectedLut;
  final String? _imagePath;
  final PreviewGeneratorChangeNotifier previewGeneratorChangeNotifier;

  @override
  Widget build(BuildContext context) {
    var imagePath = _imagePath;
    if (imagePath == null) {
      return const SizedBox.shrink();
    }
    var pathImgSelectedLut = _pathImgSelectedLut;
    if (pathImgSelectedLut == null) {
      return ExposureAnalysisWidget(imagePath, previewGeneratorChangeNotifier);
    }
    return ExposureAnalysisWidget(
        pathImgSelectedLut, previewGeneratorChangeNotifier);
  }
}

@keertip keertip added analyzer-warning Issues with the analyzer's Warning codes analyzer-ux P3 A lower priority bug or feature request labels Dec 30, 2024
@FMorschel
Copy link
Contributor

FMorschel commented Dec 30, 2024

This happens because the analyzer can't promote variables from outside the function/method. If you assign it to a local variable and then make the test for nullability, you will see it working as you expected. @lrhn or @eernstg for the explanation for this I think

@lrhn
Copy link
Member

lrhn commented Dec 30, 2024

The usual reason applies: The compiler won't promote anything that would make it a breaking change to change the implementation, or that isn't absolutely guaranteed to be stable.
A variable being final is not a promise to not change that variable to a getter, which can't be promoted, so you can't promote anything except local variables.
You'd have to make an API promise that the variable stays promotable before the compiler would trust it, which isn't currently possible.

(The one exception is a library private final variable that is not overridden inside the same library. And even then, it's only allowed to be promoted inside that library, because then the library author will immediately see if they break it.)

@stephane-archer
Copy link
Author

stephane-archer commented Dec 30, 2024

thank you @lrhn for your explanation but sadly I don't understand why a local variable can be promoted while a member variable can't (inside the function scope)

Let's take this simple example:

class _MyWidget  {
  const _MyWidget({
    required String? imagePath,
  }) : _imagePath = imagePath;

  final String? _imagePath;

   String? get hello { // the getter can be null witch is normal
    return _imagePath;
   }

  // _imagePath can't be change because of final
  //  set world (String a) {
  //   _imagePath = a;
  //  }

  int build() {
    if (_imagePath == null) {
      // I don't see why this can not be promoted like a local variable in the build scope
     // I don't see why a member function can't be treated like local variables inside a function scope
      return 1;
    }
    _bar(_imagePath);
  }

  void _bar(String a){
    print(a);
  }
}

adding at the beginning of your function this line for a null check of your member variable feels wrong:

var imagePath= _imagePath;

@FMorschel
Copy link
Contributor

FMorschel commented Dec 30, 2024

@stephane-archer see:

class A {
  const A(this.v);
  final int? v;
}

class B extends A {
  B(super.v);

  int? v2 = 0;

  @override
  int? get v => v2;

  void foo() {
    if (v == null) {
      return;
    }
    v2 = null;
    // Now v is null
  }
}

@stephane-archer
Copy link
Author

@FMorschel in class B you created an alias v for v2, there is nothing in B referencing final A.v.
if final A.v is null check it can be promoted like a local variable.

let me rewrite your code without "syntax sugar"

class A {
  const A(this.v);
  final int? v;
}

class B extends A {
  B(super.v);

  int? v2 = 0;

  void foo() {
    if (v2 != null) {
      return;
    }
    v2 = null;
    // Now v2 is null <- yes an assignment that can be null after a null check can't promote the member variable but how is this different than a local variable? 
  }
}

I don't see how an alias changes something. I think the compiler can make a difference between final A.v and B.v2 but aliased v by the user

@FMorschel
Copy link
Contributor

FMorschel commented Dec 30, 2024

This is because B.v is an extension for A.v.

Say foo was actually declared in A but with a call to another function bar.

Then in B you override bar to set the value for v2.

When you pass an instance of B in a place that is expecting A you would get a runtime error if that was promoting the property and you tried running foo (on my phone now, will edit this comment with a full example to show the above when I get to my PC).


Edit

@stephane-archer, see this updated version

void main() {
  final a1 = A(1);
  print(a1.foo());
  final a2 = B(1);
  print(a2.foo());
}

class A {
  const A(this.v);
  final int? v;

  int foo() {
    if (v == null) {
      return 0;
    }
    bar();
    return v;
  }

  void bar() {
    print('bar');
  }
}

class B extends A {
  B(super.v);

  int? v2 = 0;

  @override
  int? get v => v2;

  @override
  void bar() {
    v2 = null;
    print('bar2');
  }
}

@FMorschel
Copy link
Contributor

FMorschel commented Dec 31, 2024

A new comment to say that I've updated the comment above with the repro.

Also, about it. This can happen for example if you extend a class from a package and make one of its final fields a getter as the above. Now if the author had made a similar thing to what you asked, it would generate a runtime error.

That's what I was talking about, the safest thing to do is to create a local variable with that value and work with it instead.

@stephane-archer
Copy link
Author

@FMorschel is strange that to @override a function, the function signature should perfectly match but you can @override a final member variable by a variable type (not final).

What led you to make this decision? Right now every member variable has to be put in a local variable because of this and this added flexibility doesn't make sense for me right now. Can you name a typical use case so I can better understand?

@lrhn
Copy link
Member

lrhn commented Jan 7, 2025

is strange that to @override a function, the function signature should perfectly match but you can @override a final member variable by a variable type (not final).

That's because the API signature of a final variable is the getter signature.
That getter being backed by a variable is an implementation detail.

That's why you can freely switch between a variable and a getter - they have the same signature.
Just like you can freely switch between an async function and a plain function returning a Future. The API is the same, the rest is implementation details that should not leak through the abstraction.

@FMorschel
Copy link
Contributor

A final class propriety only means that we assign that value once and can't reassign it (it needs to be done on the constructor because of how the instantiation process works or marked as late final).

So for instances of a class with final propriety(ies), we would only have a public getter(s) for it(them) and never public setter(s).

But for a subclass, that could change. We could even add a setter to that value, it won't have any change (in signature) if being used where the original/super class was expected. But where you know this more specific type you can set the value if you wish to.

@stephane-archer
Copy link
Author

@lrhn When using class A, the fact that v is final is reflected outside by the absence of a setter but I see that final is not part of the type.

that means we have the flexibility to remove this final keyword at any time, but now we cannot use final to promote our member variable.

I'm not so sure what the value of the final keyword is if the language will treat everything as if nothing was final at the end.

Do you see any obvious drawbacks of putting final as part of the type (except it is harder to remove)?
An example that comes to mind would be const in C++. Do you see annoyance or issue with this approach?

@FMorschel
Copy link
Contributor

You are asking for a warning/lint/error to extend a property by giving it a setter on this or sub-classes? I think this is a valid request, I can see that making some sense. Maybe an annotation? Than we could "@reopen" (I think this is the annotation for final classes?) it or //ignore.

@stephane-archer
Copy link
Author

@FMorschel what are your thoughts on adding an annotation:
@sealed could enforce immutability even in subclasses, preventing redefinition or the addition of setters.
This can make final to enforce immutability across the inheritance chain.

promoting would be then possible on member variable

@FMorschel
Copy link
Contributor

I'm unsure using any language keyword is a good option. And about the sealed word specifically I'm unsure because of the meaning it has as a class modifier.

Just be aware that even with this request of discouraging adding a setter specifically (but probably not impossible, because of how OOP works), it would still be possible to override it as a getter of some sort. Maybe having yet another annotation could be then possible to discourage overrides for that getter.

CC: @srawlins @bwilkerson

@bwilkerson
Copy link
Member

... @sealed could enforce immutability even in subclasses, preventing redefinition or the addition of setters.

We could, theoretically, add an annotation that would cause a warning to be produced if a setter is ever introduced for a final field, but we couldn't use that annotation to allow promotion because such a warning isn't binding (it can be ignored or suppressed) and as a result using it to change the semantics of promotion would make the language unsound.

Given that it can't be used to change the semantics of promotion, I doubt that it carries enough value to make it worth adding.

The same line of reasoning would apply to an annotation to discourage overriding a getter.

There has been some discussion in the past of a similar idea encoded as a language feature, which would allow us to make it an error rather than a warning. See dart-lang/language#1518.

@lrhn
Copy link
Member

lrhn commented Jan 7, 2025

Do you see any obvious drawbacks of putting final as part of the type (except it is harder to remove)?

It makes it a breaking change to change a final variable to a getter.

People who have written final variables so far have not asked for that, and you would then need a new syntax to write a variable with no setter that is not that kind of final, to not make it a guaranteed final variable that you cannot convert to a getter.

Being convertible to a getter becomes opt-in, you'd have to choose to use that new syntax. If you forget, or don't know, you'll be locking yourself into a final variable that you cannot change to a get without breaking other people. Style guides would, rightly, suggest that you make all instance variables the new kind of final, and only use the promotable final variable when you really want the promotability. There might be annotations and lints to ensure that you don't make a variable final by accident.

So, just like being const, which is restricting what you can do in the future, being a "stable getter" should be the thing you opt in to. You have to show intent to restrict yourself.
Like the dart-lang/language#1518 proposal.

(This is not a new idea, it's been said many times before here and in the language issue tracker. The arguments against it haven't changed. That's why the "stable getters" proposal exists.)

@stephane-archer
Copy link
Author

but we couldn't use that annotation to allow promotion because such a warning isn't binding (it can be ignored or suppressed)

@bwilkerson just make it an error. I think this proposal is an elegant way of solving the issue. Do you have any problem with it? dart-lang/language#1518

void main() {
  final a1 = A(1);
  print(a1.foo());
  final a2 = B(1); // This line will not compile
  print(a2.foo());
}

class A {
  // Private constructor
  const A._(this.v);

  // Factory constructor for controlled instantiation
  factory A(int value) {
    return A._(value);
  }

  final int? v;

  int foo() {
    if (v == null) {
      return 0;
    }
    bar();
    return v!;
  }

  void bar() {
    print('bar');
  }
}

// Attempting to extend A
class B extends A {
  // Error: Cannot call the private constructor of A
  B(super.v); // This line will not work because A._() is private

  int? v2 = 0;

  @override
  int? get v => v2;

  @override
  void bar() {
    v2 = null;
    print('bar2');
  }
}

If we have a private constructor we can promote member variables, do you see any issue with this?

@eernstg
Copy link
Member

eernstg commented Jan 7, 2025

I'd very much like to introduce support for an interface level commitment to immutability in Dart, that is, something along the lines of dart-lang/language#1518. The main reason is that I consider immutability to be a relevant affordance which may be provided by a property of an object: You can trust this property to remain unchanged.

It is then safe to cache it in a local variable and/or promote it, but those things are just examples of the benefits that clients may obtain when they have a guarantee that a property of a given object is immutable. The point is that this property is easier to reason about in client code, which is the reason why it is not just taking something away (mutability) but also offering something in return (predictability).

@FMorschel already gave a detailed example to demonstrate that it isn't safe to promote a final instance variable. Here is an example that serves the same purpose. It's shaped in a slightly different way in order to illustrate the core point even more directly:

class A {
  final String? x;
  A(this.x);

  void test() {
    if (x != null) print(x!.length); // <--- Look here!
  }
}

class B extends A {
  bool _yieldNull = true;
  B(super.x);
  String? get x => (_yieldNull = !_yieldNull) ? null : super.x;
}

void main() {
  B('Hello').test();
}

So the claim is that "we can promote x in test because it's final, so we don't need the ! in x!.length", and the refutation is that it actually throws. Hence, promotion of x on a receiver of type A is unsafe because B can exist.

The crucial point is that evaluation of the expression x (or someA.x) is not a storage-read operation, it is a getter invocation (even in the case where it is a compiler-generated getter which was induced by an instance variable declaration). A getter can be overridden, so you can't assume that it is stable.

dart-lang/language#1518 allows us to declare that the getter must be stable. You can override or implement it using a get declaration or a variable declaration, but the declaration must be statically known to yield the same value each time it is invoked. A final variable will do that, and so will String? get x => 'Hello';, but B.x as declared above is rejected because it may return different values on different invocations.

@lrhn
Copy link
Member

lrhn commented Jan 7, 2025

I think dart-lang/language#1518 is a reasonable solution if we want to allow public fields to be promoted. (It's likely the only reasonable solution, up to syntax differences. Anything that doesn't require an explicit opt-in to enable the promotion is a no-go.)

It's also a complication to the class model and a restriction on what subclasses can do, and it's not clear it's worth its own complexity when you can just do if (v case var v?) ... use(v) today and not worry about promoting an instance variable.

That is, it's not clear that introducing complexity in the class and member model with the only benefit being that you can now (guaranteed safely) cache and promote an instance variable, is worth having every class author choose between a stable variable and just a final variable.
There would still be issues where someone asks "why doesn't this promote" when they use a normal final instance variable. If anything, it becomes even more confusing when some instance variables promote and others do not, and you have to check if the getter is stable or not. We'd likely have to introduce the notion of "stable expressions" too, as the expressions a stable getter can return, which increases the complexity even more.
Today it's easy: Instance variables don't promote. Getters may vary, it's an implementation detail whether they do or not, not something checked by the type system.

It's not that "stable properties" an unreasonable feature, or that it doesn't match object oriented modelling as such, it's just that it's a minor increment in power with a (IMO) larger increment in user-facing complexity. There are other things I'd rather spend resources on.

@eernstg
Copy link
Member

eernstg commented Jan 7, 2025

@stephane-archer wrote:

Do you have any problem with it? dart-lang/language#1518

We could have that feature, no problem. But it won't make its way to the top of the TODO list unless it gets more support. I can recommend voting for it. ;-)

@stephane-archer
Copy link
Author

Promotion of x on a receiver of type A is unsafe because B can exist.

We can prevent B from existing with a private constructor. Then the immutability is guaranteed and we can promote the member variables. Do you see any issues with this?

(Your proposal is elegant and would be useful too.)

@stephane-archer
Copy link
Author

I can recommend voting for it. ;-)

Voting is putting I thumb up? If I could I would put two ✌️

How many vote to do you need? It seems quite popular already.

@FMorschel
Copy link
Contributor

We can prevent B from existing with a private constructor.

Or by adding final to the class declaration

final class A {...}

Do you see any issues with this?

It could still be overridden I think* by extension type but that is not something people would normally do, I don't think.

*See #59855.

@eernstg
Copy link
Member

eernstg commented Jan 7, 2025

True, we can use privacy and final types to detect that an override cannot exist, and we could also use a notion of non-virtual getters to enforce that an override cannot exist.

However, even though those things are sufficient for the purpose of soundness, they don't indicate an active and conscious commitment to stability by the maintainers of this getter. That's a conceptual matter, and a matter of software engineering and modeling. It's visible in the documentation, and clients can rely on it.

I think this serves as an important motivation for having stability as an explicitly specified property, rather than as a property that may have arisen by accident (because the developers needed to make something private or final for other reasons). For example, if you rely on finality to get stability then it is a breaking change to remove the modifier final from a class declaration because it might eliminate a lot of promotions in other people's code.

@eernstg
Copy link
Member

eernstg commented Jan 7, 2025

How many vote to do you need?

There are no mechanical rules, but more is better. ;-)

@bwilkerson
Copy link
Member

but we couldn't use that annotation to allow promotion because such a warning isn't binding (it can be ignored or suppressed)

@bwilkerson just make it an error.

That's really what #59855 is about. In order for something to be an error it has to be part of the language specification. We won't define errors that are based on an annotation, so there would need to be some other mechanism, such as a new keyword, but that's just a detail.

The analyzer can report warnings that the compilers don't, but those warnings aren't part of the language and hence can't change the semantics of the language.

@stephane-archer
Copy link
Author

The approach of #59855 is great, it would be great to have a keyword for that. I'm 300% with this.

But the promotion system right now can be improved without changing the language. Promoting member variable that can not be extended is possible to add today without breaking any existing code and adding additional keyword.

It about having the analyser helping rather than been on the way by forcing me to introduce local variable while I would never extend that class to start with

@lrhn
Copy link
Member

lrhn commented Jan 8, 2025

But the promotion system right now can be improved without changing the language.

Promotion is part of the language specification. You literally cannot change promotion without changing the language.

The analyser cannot do anything about promotion by itself, the compilers decide what the program means, and they only follow the language specification.

It's (likely) correct that the language could be changed to promote some instance variables with no new syntax, but likely fewer than you think (it still has to be sound, and without a global analysis).
As explained, that's not a good language design, so it won't happen.
The only realistic way to get instance variable promotion is to have a way to declare that the variable is promotable, so the language can enforce it.

@stephane-archer
Copy link
Author

stephane-archer commented Jan 8, 2025

As explained, that's not a good language design, so it won't happen.

Could you clarify why it is bad design to promote member variable in final class A {...}?
and how var myMemberVariable = _myMemberVariable every time I want to access a member variable is a great design?

do you mean you prefer dart-lang/language#1518 solution or is it something else?

@lrhn
Copy link
Member

lrhn commented Jan 9, 2025

If you can promote a final variable of final class A{…} from another library, but not a getter, then creating a final public variable in A locks the author in to it being a final variable, and they cannot change it to a getter without breaking other libraries. That breaks variable/getter symmetry.
The author did not ask for that, they only asked for a way to store a value. There would need to be a way to declare a variable that was not promotable, so that the author keeps the option of changing it to a getter in the future.
In fact, that should be the default, because otherwise an author can accidentally lock themselves in to a promotable variable. Just like being constant is an API promise that you have to explicitly make, being stable should be something you declare, not something the compiler does "if it can".

Which is another way of saying that final int foo; must not be promotable from other libraries, the author needs to write something more to opt-in to promotability.
So yes, I prefer something like the stable getters proposal.

The only place where promoting based on implementation, not signature, is acceptable is inside the same library.
It still requires proving that it's completely impossible that the getter can currently be implemented by anything other than a final variable, which means that it's completely impossible for other libraries to implement the same interface, or if the variable is accessed on this, for other libraries to extend the class.
That's what is currently done for some private final instance variables. If there is no getter with the same name, implicitly or explicitly, inside the same library, then the compiler knows that there won't be any added in other libraries, and the final private instance variable can be promoted … if the compiler can see that it's definitely read from the same object.

It may be possible to do the same thing with non-private instance variables if the class, and all subclasses, are either final or sealed. It would still only allow promotion inside the same library, because then it's immediately locally visible if you change something and break promotion. (Can potentially allow a subclass to be only base, but only if it's library private. Then the compiler also had to check that that declaration is not leaked through a public type alias. Easier to just require it to be final, it makes no difference inside the same library.)

It should be possible to promote static and top level final variables in the same library too, but you rarely need to test the type of those.

It's so restrictive (same library only, all subclasses final, final instance variables only) that it may not be useful enough to be worth the effort.
@stereotype441

@stephane-archer
Copy link
Author

Just like being constant of an API promise that you have to explicitly make, being stable should be something you declare, not something the compiler does "if it can".

I see your point

@lrhn lrhn removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-ux analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-question A question about expected behavior or functionality
Projects
None yet
Development

No branches or pull requests

7 participants