dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

Bug: false alarm of the analyzer #54753

Closed ChaserVasya closed 8 months ago

ChaserVasya commented 8 months ago

There is a code snippet.

class Date extends DateTime {
  factory Date.from(DateTime other) {
    return ((other.isUtc) ? Date.utc : Date.new)(
      other.year,
      other.month,
      other.day,
    );
  }

  Date(super.year, [super.month, super.day]);
  Date.utc(super.year, [super.month, super.day]) : super.utc();

  Date copyWith({
    int? year,
    int? month,
    int? day,
    bool? isUtc,
  }) {
    // ignore: unnecessary_cast
    return (this as DateTime)
        .copyWith(
          year: year,
          month: month,
          day: day,
          isUtc: isUtc,
        ).clipTime;
    }
}

extension ClipTimeDateTime on DateTime {
  Date get clipTime => Date.from(this);
}

There are 2 copyWith: 1) extension DateTimeCopyWith on DateTime 2) Date.copyWith method.

Date.copyWith should use DateTime.copyWith. Analyzer alerts 'Unnecessary cast'. I remove (this as DateTime). Then there is a cycle dependency:Date.copyWith uses Date.copyWith:

  Date copyWith({
    int? year,
    int? month,
    int? day,
    bool? isUtc,
  }) {
    return copyWith(
          year: year,
          month: month,
          day: day,
          isUtc: isUtc,
        ).clipTime;
    }

It is wrong behaviour.

Also I can't use super.copyWith: The method 'copyWith' isn't defined in a superclass of 'Date'.

Desired behavior

Analyzer doesn't alert unnesessary_cast in this scenario.

General info

Project info

Process info

Memory CPU Elapsed time Command line
629 MB 0.0% 16:10 dart language-server --client-id=Android-Studio --client-version=AI-222.4459.24 --protocol=analyzer
60 MB 0.0% 16:11 flutter_tools.snapshot daemon
eernstg commented 8 months ago

unnecessary_cast does indeed have false positives. However, in this particular situation you'd probably do the following anyway (which means that there is no cast, and hence no spurious messages from unnecessary_cast):

  Date copyWith({
    int? year,
    int? month,
    int? day,
    bool? isUtc,
  }) {
    return DateTimeCopyWith(this) // <-- Select the extension explicitly.
        .copyWith(
          year: year,
          month: month,
          day: day,
          isUtc: isUtc,
        )
        .clipTime;
  }

The point is that you can guide the resolution of an extension member invocation by modifying the type of the receiver (e.g., using a cast), but if you just want to ensure that the invocation will call a specific extension member then you might just as well perform the resolution explicitly. This will document the purpose much more directly.

This technique will force the resolution to choose the extension member that you want to invoke, even in the case where some of the conflicting declarations are modified (there are some conflicting declarations: you never need to influence the resolution of an extension member invocation unless there are other declarations with the same name, e.g., instance members and/or other extension members). This means that the manually resolved extension method invocation is more robust over time as your software (and the software that you import) evolves.

Of course, it's much more convenient if there is no need to specify the extension explicitly, but it is probably not possible to completely avoid this kind of name clash.

eernstg commented 8 months ago

@ChaserVasya, I think I should close this issue because everything except possibly the unnecessary_cast lint is working as intended. I don't see the lint issue locally (though I haven't tried to debug that), but it would be great if you can create a concise example and report it on the linter repository.