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

SIM300: CONSTANT_CASE variables are improperly flagged for yoda violation #9164

Merged

Conversation

asafamr-mm
Copy link
Contributor

@asafamr-mm asafamr-mm commented Dec 16, 2023

Summary

fixes #6956
details in issue

Following an advice in #6956 (comment),
this change separates expressions to 3 levels of "constant likelihood":

  • literals, empty dict and tuples... (definitely constant, level 2)
  • CONSTANT_CASE vars (probably constant, 1)
  • all other expressions (0)

a comparison is marked yoda if the level is strictly higher on its left hand side

following #6956 (comment) marking compound expressions of literals (e.g. 60 * 60 ) as constants
this change current behaviour on SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) in the fixture from error to ok

Copy link
Contributor

github-actions bot commented Dec 16, 2023

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+15 -0 violations, +0 -0 fixes in 4 projects; 37 projects unchanged)

apache/airflow (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-preview --select ALL

+ tests/plugins/test_plugins_manager.py:146:16: SIM300 [*] Yoda conditions are discouraged
+ tests/providers/cncf/kubernetes/executors/test_kubernetes_executor.py:156:20: SIM300 [*] Yoda conditions are discouraged
+ tests/providers/cncf/kubernetes/test_pod_generator.py:579:16: SIM300 [*] Yoda conditions are discouraged, use `result.metadata.annotations["dag_id"] == "a" * 512` instead
+ tests/providers/cncf/kubernetes/test_pod_generator.py:580:16: SIM300 [*] Yoda conditions are discouraged

demisto/content (+5 -0 violations, +0 -0 fixes)

+ Packs/FeedCyjax/Integrations/FeedCyjax/FeedCyjax_test.py:20:12: SIM300 [*] Yoda conditions are discouraged, use `INDICATORS_LAST_FETCH_KEY == 'last_fetch'` instead
+ Packs/FeedCyjax/Integrations/FeedCyjax/FeedCyjax_test.py:21:12: SIM300 [*] Yoda conditions are discouraged, use `DATE_FORMAT == '%Y-%m-%dT%H:%M:%SZ'` instead
+ Packs/FeedCyjax/Integrations/FeedCyjax/FeedCyjax_test.py:22:12: SIM300 [*] Yoda conditions are discouraged, use `INDICATORS_LIMIT == 50` instead
+ Tests/scripts/wait_until_server_ready.py:104:28: SIM300 [*] Yoda conditions are discouraged, use `SETUP_TIMEOUT != 60 * 60` instead
+ Tests/scripts/wait_until_server_ready.py:96:28: SIM300 [*] Yoda conditions are discouraged, use `SETUP_TIMEOUT != 60 * 10` instead

reflex-dev/reflex (+1 -0 violations, +0 -0 fixes)

+ reflex/app_module_for_backend.py:7:4: SIM300 [*] Yoda conditions are discouraged, use `constants.CompileVars.APP != "app"` instead

zulip/zulip (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-preview --select ALL

+ zerver/views/realm_emoji.py:47:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/realm_icon.py:25:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/realm_logo.py:31:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/upload.py:318:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/user_settings.py:413:8: SIM300 [*] Yoda conditions are discouraged

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM300 15 15 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+474 -2 violations, +0 -0 fixes in 8 projects; 33 projects unchanged)

apache/airflow (+359 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ dev/breeze/tests/test_packages.py:112:12: SIM300 [*] Yoda conditions are discouraged
+ dev/breeze/tests/test_packages.py:117:12: SIM300 [*] Yoda conditions are discouraged
+ dev/breeze/tests/test_packages.py:122:12: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_cleanup_pods.py:182:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_cleanup_pods.py:226:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_cleanup_pods.py:229:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_cleanup_pods.py:240:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:164:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:179:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:193:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:206:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:209:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:335:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:336:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_create_user_job.py:356:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_logs_persistent_volume_claim.py:66:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:155:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:179:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:217:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:231:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:244:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:247:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:317:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_migrate_database_job.py:318:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_pod_launcher_role.py:51:20: SIM300 [*] Yoda conditions are discouraged, use `docs == []` instead
+ helm_tests/airflow_aux/test_pod_template_file.py:537:16: SIM300 [*] Yoda conditions are discouraged, use `jmespath.search("spec.affinity", docs[0]) == {}` instead
+ helm_tests/airflow_aux/test_pod_template_file.py:656:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_pod_template_file.py:674:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_pod_template_file.py:686:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_pod_template_file.py:734:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_pod_template_file.py:751:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_pod_template_file.py:91:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_core/test_dag_processor.py:108:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_core/test_dag_processor.py:126:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_core/test_dag_processor.py:336:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_core/test_dag_processor.py:371:16: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_core/test_dag_processor.py:420:16: SIM300 [*] Yoda conditions are discouraged
... 322 additional changes omitted for project

bokeh/bokeh (+15 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ tests/unit/bokeh/application/handlers/test_server_request_handler.py:121:16: SIM300 [*] Yoda conditions are discouraged
+ tests/unit/bokeh/document/test_document.py:717:16: SIM300 [*] Yoda conditions are discouraged, use `foos == [42,43]` instead
- tests/unit/bokeh/io/test_doc.py:54:12: SIM300 [*] Yoda conditions are discouraged, use `[] == bid._PATCHED_CURDOCS` instead
+ tests/unit/bokeh/models/test_sources.py:100:16: SIM300 [*] Yoda conditions are discouraged, use `list(ds.data['index']) == [0, 1]` instead
+ tests/unit/bokeh/models/test_sources.py:112:16: SIM300 [*] Yoda conditions are discouraged, use `list(ds.data['index']) == [0, 1]` instead
+ tests/unit/bokeh/models/test_sources.py:124:16: SIM300 [*] Yoda conditions are discouraged, use `list(ds.data['level_0']) == [0, 1]` instead
+ tests/unit/bokeh/models/test_sources.py:138:16: SIM300 [*] Yoda conditions are discouraged, use `list(ds.data['level_0']) == [0, 1]` instead
+ tests/unit/bokeh/models/test_sources.py:151:16: SIM300 [*] Yoda conditions are discouraged, use `list(ds.data['index']) == [0, 1]` instead
+ tests/unit/bokeh/models/test_sources.py:166:16: SIM300 [*] Yoda conditions are discouraged, use `list(ds.data['index']) == [0, 1]` instead
+ tests/unit/bokeh/models/test_sources.py:86:16: SIM300 [*] Yoda conditions are discouraged, use `list(ds.data['index']) == [0, 1]` instead
... 7 additional changes omitted for project

demisto/content (+89 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ Packs/Algosec/Scripts/AlgosecGetTicket/AlgosecGetTicket_test.py:18:12: SIM300 [*] Yoda conditions are discouraged, use `content == {'some_info': 'info: test'}` instead
+ Packs/ApiModules/Scripts/CSVFeedApiModule/CSVFeedApiModule_test.py:241:20: SIM300 [*] Yoda conditions are discouraged, use `indicators[0]['fields']['tags'] == []` instead
+ Packs/ApiModules/Scripts/HTTPFeedApiModule/HTTPFeedApiModule_test.py:169:12: SIM300 [*] Yoda conditions are discouraged, use `client.get_feed_config() == {}` instead
+ Packs/AzureNetworkSecurityGroups/Integrations/AzureNetworkSecurityGroups/AzureNetworkSecurityGroups_test.py:73:12: SIM300 [*] Yoda conditions are discouraged
+ Packs/Base/Scripts/GetIncidentsByQuery/GetIncidentsByQuery_test.py:152:12: SIM300 [*] Yoda conditions are discouraged, use `entry['Contents'][0]['context'] == {}` instead
+ Packs/CarbonBlackProtect/Scripts/CBPCatalogFindHash/CBPCatalogFindHash_test.py:14:12: SIM300 [*] Yoda conditions are discouraged
+ Packs/CheckPointSandBlast/Integrations/CheckPointSandBlast/CheckPointSandBlast_test.py:122:12: SIM300 [*] Yoda conditions are discouraged
+ Packs/CiscoASA/Integrations/CiscoASA/CiscoASA_test.py:129:12: SIM300 [*] Yoda conditions are discouraged, use `command_results.outputs == []` instead
+ Packs/CommonScripts/Scripts/DomainReputation/DomainReputation_test.py:34:12: SIM300 [*] Yoda conditions are discouraged, use `results_mock.call_args[0][0] == []` instead
+ Packs/CommonScripts/Scripts/ExtractIndicatorsFromTextFile/ExtractIndicatorsFromTextFile_test.py:27:12: SIM300 [*] Yoda conditions are discouraged
... 79 additional changes omitted for project

model-bakers/model_bakery (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ tests/test_baker.py:800:16: SIM300 [*] Yoda conditions are discouraged, use `instance.fk == ["foo"]` instead

reflex-dev/reflex (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ reflex/app_module_for_backend.py:7:4: SIM300 [*] Yoda conditions are discouraged, use `constants.CompileVars.APP != "app"` instead

rotki/rotki (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ rotkehlchen/tests/db/test_db_upgrades.py:580:12: SIM300 [*] Yoda conditions are discouraged, use `[x[0] for x in manual_balance_ids] == [1, 2, 3]` instead
+ rotkehlchen/tests/unit/test_eth2.py:760:16: SIM300 [*] Yoda conditions are discouraged, use `hidden_ids == [2]` instead

sphinx-doc/sphinx (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ sphinx/ext/napoleon/docstring.py:1236:17: SIM300 [*] Yoda conditions are discouraged, use `[line1, line2] == ['', '']` instead
+ sphinx/util/__init__.py:205:16: SIM300 [*] Yoda conditions are discouraged, use `begend == ['', '']` instead

zulip/zulip (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ zerver/views/realm_emoji.py:47:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/realm_icon.py:25:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/realm_logo.py:31:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/upload.py:318:8: SIM300 [*] Yoda conditions are discouraged
+ zerver/views/user_settings.py:413:8: SIM300 [*] Yoda conditions are discouraged

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM300 476 474 2 0 0

}

/// Return [`Expr`] [`ConstantLikelihood`] level depending on simple heuristics.
fn how_likely_constant(expr: &Expr) -> ConstantLikelihood {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably add an associated method on ConstantLikelihood, like:

impl ConstantLikelihood {
  fn from_expression(expr: &Expr) -> Self { ... }
}

It helps with discoverability, since this is kind of a constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this?

impl ConstantLikelihood {
fn from_expression(expr: &Expr, is_preview_enabled: bool) -> Self {
match expr {
_ if expr.is_literal_expr() => ConstantLikelihood::Definitely,
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
ConstantLikelihood::from_identifier(attr)
}
Expr::Name(ast::ExprName { id, .. }) => ConstantLikelihood::from_identifier(id),
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts
.iter()
.map(|x| ConstantLikelihood::from_expression(x, is_preview_enabled))
.min()
.unwrap_or(ConstantLikelihood::Definitely),
Expr::List(ast::ExprList { elts, .. }) if is_preview_enabled => elts
.iter()
.map(|x| ConstantLikelihood::from_expression(x, is_preview_enabled))
.min()
.unwrap_or(ConstantLikelihood::Definitely),
Expr::Dict(ast::ExprDict { values: vs, .. }) if is_preview_enabled => {
if vs.is_empty() {
ConstantLikelihood::Definitely
} else {
ConstantLikelihood::Probably
}
}
Expr::BinOp(ast::ExprBinOp { left, right, .. }) => cmp::min(
ConstantLikelihood::from_expression(left, is_preview_enabled),
ConstantLikelihood::from_expression(right, is_preview_enabled),
),
Expr::UnaryOp(ast::ExprUnaryOp {
op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert,
operand,
range: _,
}) => ConstantLikelihood::from_expression(operand, is_preview_enabled),
_ => ConstantLikelihood::Unlikely,
}
}
fn from_identifier(identifier: &str) -> Self {
if str::is_cased_uppercase(identifier) {
ConstantLikelihood::Probably
} else {
ConstantLikelihood::Unlikely
}
}
}
/// Generate a fix to reverse a comparison.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, exactly!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we didn't need preview, we could also do...

impl From<&Expr> for ConstantLikelihood {
  ...
}

That would enable us to do things like expr.into().

Expr::Name(ast::ExprName { id, .. }) => str::is_cased_uppercase(id),
Expr::Attribute(ast::ExprAttribute { attr, .. }) => how_likely_constant_str(attr),
Expr::Name(ast::ExprName { id, .. }) => how_likely_constant_str(id),
Expr::Tuple(ast::ExprTuple { elts, .. }) | Expr::List(ast::ExprList { elts, .. }) => elts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider omitting lists and dictionaries, or at the very least they should be added under preview mode to avoid new violations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I guess the use of list was actually the motivating issue here. So, alternatively, can we try gating this whole change under preview, to avoid disruption for folks on stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved lists and dicts to preview

}
Expr::BinOp(ast::ExprBinOp { left, right, .. }) => {
cmp::min(how_likely_constant(left), how_likely_constant(right))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

fn how_likely_constant(expr: &Expr) -> ConstantLikelihood {
if expr.is_literal_expr() {
return ConstantLikelihood::Definitely;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entirely personal style but I'd probably just put this in the match with all the cases rather than adding an early return. I just find it a bit clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to correct me, I'm here to learn 🙂
Would you say this is idiomatic:

match expr {
_ if expr.is_literal_expr() => ConstantLikelihood::Definitely,
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {

(trying to DRY this invocation from multiple patterns)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think what you have here with the match is a nice approach.

enum ConstantLikelihood {
Unlikely = 0,
Probably = 1, // CAMEL_CASED vars
Definitely = 2, // literals, empty dicts and tuples...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: make these rustdocs? Like:

/// The expression is unlikely to be a constant (e.g., `foo` or `foo(bar)`).
Unlikely = 0
/// The expression is likely to be a constant (e.g., `FOO`).
Probably = 1,
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. feel free to correct my English there...

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a great change!

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent, thank you!

@charliermarsh charliermarsh force-pushed the SIM300-CONSTANT-CASE-false-positives branch from 3952ad9 to 0a0b042 Compare December 20, 2023 17:35
@charliermarsh charliermarsh enabled auto-merge (squash) December 20, 2023 17:36
@charliermarsh charliermarsh added bug Something isn't working rule Implementing or modifying a lint rule labels Dec 20, 2023
@charliermarsh charliermarsh merged commit 5d41c84 into astral-sh:main Dec 20, 2023
16 checks passed
mikaelarguedas added a commit to mikaelarguedas/ruff that referenced this pull request Dec 28, 2023
…ect's constant

follow-up of fix added in astral-sh#9164
Signed-off-by: Mikael Arguedas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIM300: CONSTANT_CASE variables are improperly flagged for yoda violation
2 participants