dart-code-checker / dart-code-metrics

Software analytics tool that helps developers analyse and improve software quality.
https://dcm.dev
Other
860 stars 265 forks source link

[BUG]: check-unnecessary-nullable check false positive result in Tear-offs #1153

Closed MorosophDude closed 1 year ago

MorosophDude commented 1 year ago

Please show your full configuration:

Configuration ```yaml include: package:lint/analysis_options.yaml linter: rules: constant_identifier_names: false analyzer: exclude: - "**/*.g.dart" - "**/*.freezed.dart" errors: invalid_annotation_target: ignore use_super_parameters: ignore ```

What did you do? Please include the source code example causing the issue.

All I did was run CLI command to check for unnecessary nullable

 dart run dart_code_metrics:metrics check-unnecessary-nullable lib

More Detail trying to make sense on why and what actually happened

So I have a function that validates a form and add/update an item in a list

bool validateAndAdd({int? indexOfItemToBeUpdated}) {
 // some validation logic
}

Here, if the indexOfItemToBeUpdated is null then the item will be added else item at provided index will be updated with new values.

This is how these values are called/used

// For Add
openEditorDialog(
    onSubmit: addEditController.validateAndAdd, // Notice this line
    child: const AddEditView(),
);

// For Update
openEditorDialog(
    onSubmit: () => addEditController.validateAndAdd(indexOfItemToBeUpdated: index),
    child: const AddEditView(),
);

So what is the problem? Running

 dart run dart_code_metrics:metrics check-unnecessary-nullable lib

gives

lib/app/.../my_file_name.dart:
    method validateAndAdd has unnecessary nullable parameters
          (int? indexOfItemToBeUpdated)
          at /Users/.../my_file_name.dart:46:3

Anything else? I change how validateAndAdd function was called for Add case I just replaced Tear-off method of calling a function with equivalent expression function

// Using Tear-off (Old)
// openEditorDialog(
//     onSubmit: addEditController.validateAndAdd,
//     child: const AddEditView(),
// );

// Using expression function (New)
openEditorDialog(
    onSubmit: () => addEditController.validateAndAdd(),
    child: const AddEditView(),
);

and then I ran check-unnecessary-nullable command once more. Voila! dart_code_metrics doesn't list this file anymore

Final Thought I love using tear-offs and it would be great if dart_code_metrics supports them. This example provided above is just 1 of many places I have used tear-off and got this message. I know this might not be priority task compared to some other Open tickets and that is fine but please have a look at this when you get a chance.

Are you willing to submit a pull request to fix this bug? No. I would love to help but I don't know where/how to start.

incendial commented 1 year ago

This example provided above is just 1 of many places I have used tear-off and got this message.

@MorosophDude hi, thank you for reporting the issue. Do all the places you mentioned result in the same problem or there are multiple problems with tear-offs?

MorosophDude commented 1 year ago

All of them have the same problem

incendial commented 1 year ago

Can you provide the type of onSubmit?

MorosophDude commented 1 year ago

Sure it has a return type of bool

Here is the function definition just in case

void openEditorDialog({
  required Widget child,
  required bool Function() onSubmit,
}) {
    // Logic to show Dialog, wrapped with WillPopScope
}

One more thing, I know it is not related in anyway but you should know I am using GetX (get: ^4.6.5)

incendial commented 1 year ago

@MorosophDude the fix is already in the master branch, can you test it?

One more thing, I know it is not related in anyway but you should know I am using GetX (get: ^4.6.5)

I don't think I can use this info anyhow, so, yeah, not related πŸ™‚

MorosophDude commented 1 year ago

I tested the fix from master branch, and it kinda worked.

Here are the things I did.

After reading your comment I checked for new version of dart_code_metrics and saw

feat: ignore tear-off methods for avoid-unused-parameters.

in changelog of 5.4.0. (So, Step 1 and 2 just to be sure I was using the latest thing)

Step 1: flutter pub cache clean Step 2: flutter pub get

This didn't change the result, it still showed issues in 10 files.

Remembered that master branch and the branch for pub.dev could be different. So went ahead and changed pubspec.yaml

Step 3: Replaced from

dev_dependencies:
  dart_code_metrics: ^5.4.0

to

dev_dependencies:
  dart_code_metrics:
    git:
      url: https://github.com/dart-code-checker/dart-code-metrics.git
      ref: master

This changed the result. Now it shows issues in 1 file.

Is this remaining 1 file different than other in any way? Yes, while other 9 called the tear-off like this

openEditorDialog(
    onSubmit: addEditController.validateAndAdd,
    child: const AddEditView(),
);

this one had it called like this

openEditorDialog(
    onSubmit: employeeAddEditController
                      .employeeWorkBreakdownAddEditController
                      .validateAndAdd,
    child: const AddEditView(),
);

All other things are same, the only difference being nesting/chain of controllers.

In the mean time I will try to replicate this without using controllers (just plain function that is in the same file), if I succeed I will post it here.

incendial commented 1 year ago

feat: ignore tear-off methods for avoid-unused-parameters.

This one is about the rule, not the command πŸ™‚.

onSubmit: employeeAddEditController .employeeWorkBreakdownAddEditController .validateAndAdd,

Interesting, never though this syntax is actually possible πŸ˜…. (update: I misunderstood the syntax πŸ˜…) I'll also test this case, thank you!

incendial commented 1 year ago

I've merged another fix, please try again

MorosophDude commented 1 year ago

Terribly sorry for the late reply. It is weekend and I couldn't log-in to GitHub on my personal device.

Anyway here is my finding:

PS C:\Users\myUsername\path_to_my_project> flutter pub run dart_code_metrics:metrics check-unnecessary-nullable lib
βœ” Analysis is completed. Preparing the results: 45.8s

βœ” no unnecessary nullable parameters found!

Why did I run flutter pub run dart_code_metrics:... now, when I was running dart run dart_code_metrics:... previously? Well it didn't work.

PS C:\Users\my_username\path_to_my_project> dart run dart_code_metrics:metrics check-unnecessary-nullable lib       
Could not find a file named "pubspec.yaml" in "C:\Users\my_username\AppData\Local\Pub\Cache\hosted\pub.dartlang.org\_fe_analyzer_shared-50.0.0".
#0      new Pubspec.load (package:pub/src/pubspec.dart:306:7)
#1      new Package.load (package:pub/src/package.dart:128:29)
#2      SystemCache.load (package:pub/src/system_cache.dart:105:20)
#3      Entrypoint._createPackageGraph (package:pub/src/entrypoint.dart:121:63)
#4      Entrypoint.packageGraph (package:pub/src/entrypoint.dart:116:54)
#5      getExecutableForCommand (package:pub/src/executable.dart:339:19)
#6      RunCommand.run (package:dartdev/src/commands/run.dart:250:32)
#7      CommandRunner.runCommand (package:args/command_runner.dart:209:27)
#8      DartdevRunner.runCommand (package:dartdev/dartdev.dart:231:30)
#9      CommandRunner.run.<anonymous closure> (package:args/command_runner.dart:119:25)
#10     new Future.sync (dart:async/future.dart:302:31)
#11     CommandRunner.run (package:args/command_runner.dart:119:14)
#12     runDartdev (package:dartdev/dartdev.dart:66:29)
#13     main (file:///C:/b/s/w/ir/x/w/sdk/pkg/dartdev/bin/dartdev.dart:11:9)
#14     _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:293:32)
#15     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:192:12)

I am pretty sure everything is setup correctly in my laptop. This might be some other existing issue on Windows, I will have a look at other open issue and make a new one if I don't find anything relatable.

incendial commented 1 year ago

Terribly sorry for the late reply.

Not a problem at all! Thank you for testing, looks like it's finally working properly.

Well it didn't work.

Yeah, looks pretty strange, considering that from our side nothing significant has changed...

MorosophDude commented 1 year ago

Yeah, looks pretty strange, considering that from our side nothing significant has changed...

Oh, I wasn't clear. This happens on 5.4.0 as well(didn't check on older versions). I was back at office today and tested on mac and everything is working fine here but tried the same thing on Windows machine of a colleague and dart run dart_code_metrics:... is not working while flutter pub run dart_code_metrics:... works fine.

I didn't find any open issue for this so maybe there is something wrong on my side. I will do some more testing possibly on some other windows devices and with different versions of dart_code_metrics. I will create a new Issue once I have enough info on what is going on.

As for this Issue should I close it or will you close it once 5.5.0 gets released (I am not familiar with GitHub mannerism).

incendial commented 1 year ago

As for this Issue should I close it or will you close it once 5.5.0 gets released (I am not familiar with GitHub mannerism).

I'll close it when the fix will be released.

I didn't find any open issue for this so maybe there is something wrong on my side. I will do some more testing possibly on some other windows devices and with different versions of dart_code_metrics. I will create a new Issue once I have enough info on what is going on.

Yeah, looks a bit strange, but from the stack trace you've provided - it's the problem with the dependency resolution and the discussion in this issue https://github.com/dart-lang/sdk/issues/49167 might help

MorosophDude commented 1 year ago

the discussion in this issue https://github.com/dart-lang/sdk/issues/49167 might help

Thanks, I will look into it

incendial commented 1 year ago

Fixed in 5.5.0 πŸš€