Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

[BUG] check-unused-l10n doesn't find methods called via getter #570

Closed
furaiev opened this issue Nov 24, 2021 · 19 comments
Closed

[BUG] check-unused-l10n doesn't find methods called via getter #570

furaiev opened this issue Nov 24, 2021 · 19 comments
Assignees
Labels
area-unused-l10n type: bug Something isn't working

Comments

@furaiev
Copy link
Contributor

furaiev commented Nov 24, 2021

  • Dart code metrics version: 4.7.0
  • Dart sdk version: 2.14.4 (stable)

Please show your full configuration:

Configuration
analyzer:
  exclude:
    - "**/*.g.dart"
    - "**/*.gr.dart"
    - "**/*.freezed.dart"
    - "**/generated/**.dart"
    - "**/*.gform.dart"
    - "ios/**"
  strong-mode:
    implicit-casts: false
    implicit-dynamic: false
  errors:
    missing_required_param: warning
    missing_return: error
  plugins:
    - dart_code_metrics

dart_code_metrics:
  anti-patterns:
    - long-method
    - long-parameter-list
  metrics:
    cyclomatic-complexity: 27
    source-lines-of-code: 50
    number-of-parameters: 4
    maximum-nesting-level: 5
  metrics-exclude:
    - test/**
    - e2e/**
    - ios/**
  rules:
    - always-remove-listener
    - avoid-non-null-assertion
    - avoid-returning-widgets
    - avoid-unnecessary-setstate
    - avoid-unused-parameters
    - avoid-wrapping-in-padding
    - double-literal-format
    - newline-before-return
    - no-boolean-literal-compare
    - prefer-match-file-name:
        exclude:
          - test/**
    - prefer-single-widget-per-file
    - prefer-trailing-comma
    - no-equal-then-else
    - binary-expression-operand-order

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

flutter pub run dart_code_metrics:metrics check-unused-l10n lib -p ^S$

Code details

class S {
  S();

  static S? _current;

  static S get current {
    assert(_current != null,
        'No instance of S was loaded. Try to initialize the S delegate before accessing S.current.');
    return _current!;
  }
…
}

What did you expect to happen?
the command consider S.current.title as used l10n

What actually happened?
the command doesn't consider S.current.title as used l10n

@dkrutskikh dkrutskikh added area-unused-l10n type: enhancement New feature or request type: bug Something isn't working and removed type: enhancement New feature or request labels Nov 24, 2021
@diegoveloper
Copy link
Contributor

Same issue with extensions:

extension L10nExtensions on BuildContext {
  MyLocalization get l10n => MyLocalization.of(this);
}

If we call :

final myTitle = context.l10n.title;

That's not considered as used i10n.

@incendial
Copy link
Member

@diegoveloper that looks harder, could you share, where MyLocalization localization strings are placed in your approach?

@incendial
Copy link
Member

Actually, sorry, I was wrong, we don't need this info to calculate usages correctly.

@incendial
Copy link
Member

I'll take a closer look a bit later.

@incendial incendial added the waiting for release Will be available after new version is released label Jan 9, 2022
@dkrutskikh dkrutskikh added this to the 4.9.0 milestone Jan 10, 2022
@dkrutskikh
Copy link
Member

Available in 4.9.0 release 🚀

@incendial incendial removed the waiting for release Will be available after new version is released label Jan 10, 2022
@furaiev
Copy link
Contributor Author

furaiev commented Jan 11, 2022

Thanks, guys,
Can't fully check it due to #524 (comment)
But seems like works for getters.

I've found that it doesn't work for functions.

  String amountWithCurrency(Object arg1) {
    return Intl.message(
      '\$$arg1',
      name: 'amountWithCurrency',
      desc: '',
      args: [arg1],
    );
  }
  
  // and then
  S.current.amountWithCurrency(value);

@incendial
Copy link
Member

@furaiev could you share the code example for the case it's not working as expected?

@furaiev
Copy link
Contributor Author

furaiev commented Jan 11, 2022

// .arb

  "popupMediaAccessTitle": "Allow access to {arg1}",
  "@popupMediaAccessTitle": {
    "type": "text",
    "placeholders": {
      "arg1": {}
    }
  },


// generated .dart

class S {
  S();

  static S? _current;

  static S get current {
    assert(_current != null,
        'No instance of S was loaded. Try to initialize the S delegate before accessing S.current.');
    return _current!;
  }

  static const AppLocalizationDelegate delegate = AppLocalizationDelegate();

  static Future<S> load(Locale locale) {
    final name = (locale.countryCode?.isEmpty ?? false)
        ? locale.languageCode
        : locale.toString();
    final localeName = Intl.canonicalizedLocale(name);
    return initializeMessages(localeName).then((_) {
      Intl.defaultLocale = localeName;
      final instance = S();
      S._current = instance;

      return instance;
    });
  }

  static S of(BuildContext context) {
    final instance = S.maybeOf(context);
    assert(instance != null,
        'No instance of S present in the widget tree. Did you add S.delegate in localizationsDelegates?');
    return instance!;
  }

  static S? maybeOf(BuildContext context) {
    return Localizations.of<S>(context, S);
  }

  /// `Allow access to {arg1}`
  String popupMediaAccessTitle(Object arg1) {
    return Intl.message(
      'Allow access to $arg1',
      name: 'popupMediaAccessTitle',
      desc: '',
      args: [arg1],
    );
  }

// ...
}


// usage 
S.current.popupMediaAccessTitle('camera');

@incendial incendial reopened this Jan 12, 2022
@incendial
Copy link
Member

@furaiev thank you, I'll try to reproduce it today and add a fix.

@furaiev
Copy link
Contributor Author

furaiev commented Jan 12, 2022

@incendial thank you, could you please also reopen #524

@incendial incendial modified the milestones: 4.9.0, 4.10.0 Jan 12, 2022
@incendial
Copy link
Member

incendial commented Jan 12, 2022

@furaiev are you sure it's not just this problem? From what I see it's because of current usage. Or of(context). popupMediaAccessTitle() is broken too?

@furaiev
Copy link
Contributor Author

furaiev commented Jan 12, 2022

There are broken getters (w/o arguments) via current, e.g. S.of(context).loginTitle

Summary:
S.current.popupMediaAccessTitle('camera'); // does't work
S.of(context).popupMediaAccessTitle('camera'); // work
S.of(context).loginTitle; // doesn't work

@incendial
Copy link
Member

@furaiev interesting, but tests don't trigger on that. Could you share loginTitle declaration?

@incendial
Copy link
Member

Got it, working on a fix.

@incendial incendial added the waiting for release Will be available after new version is released label Jan 12, 2022
@dkrutskikh
Copy link
Member

Available in 4.9.1 release 🚀

@incendial incendial removed the waiting for release Will be available after new version is released label Jan 13, 2022
@incendial incendial removed this from the 4.10.0 milestone Jan 13, 2022
@winsonloh1998
Copy link

I just updated it to version 4.9.1 and I still get the same issue, and probably my way of calling the getter is different.

This is how get the localized text:

@override
Widget build(BuildContext context) {
   final _sDelegate = S.of(context);
    
   return Text(_sDelegate.my_text);
}

For this case, the command recognised it as unused too.

@furaiev
Copy link
Contributor Author

furaiev commented Jan 14, 2022

@dkrutskikh @incendial thank you for a fix, all is works for me.

Could you please exclude private members of the Localization class? (this is not critical at all, just nice to have)

class S {
  S();

  static S? _current;  // <<<<<<<<<< exclude this one 

  static S get current {
    assert(_current != null,
        'No instance of S was loaded. Try to initialize the S delegate before accessing S.current.');
    return _current!;
  }
...

@winsonloh1998 the example that you have provided is another issue/feature(?). I'm not sure if it is easy to manage and track Localization reassignments. But let's wait for @dkrutskikh @incendial answer.

@incendial
Copy link
Member

incendial commented Jan 14, 2022

@winsonloh1998 @furaiev could you create separate issues for you feature requests? Both of them look valid and nice to have, but they are out of this issue scope.

@furaiev
Copy link
Contributor Author

furaiev commented Jan 14, 2022

#647
#648

@incendial @winsonloh1998

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-unused-l10n type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants