Skip to content

Conversation

@InSyncWithFoo
Copy link
Contributor

@InSyncWithFoo InSyncWithFoo commented Jun 17, 2025

Summary

Part of #111.

After this change, dataclasses with two or more KW_ONLY field will be reported as invalid. The duplicate fields will simply be ignored when computing __init__'s signature.

Test Plan

Markdown tests.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2025

mypy_primer results

No ecosystem changes detected ✅

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Jun 17, 2025
@InSyncWithFoo InSyncWithFoo force-pushed the ty-dataclasses-duplicate-kw-only branch from bab19ad to c93c828 Compare June 18, 2025 09:58
@InSyncWithFoo InSyncWithFoo marked this pull request as ready for review June 18, 2025 21:45
@InSyncWithFoo
Copy link
Contributor Author

Currently the diagnostic doesn't look quite as nice as I want it to be. I think it should show all offending fields in subdiagnostics, but .fields() doesn't contain AST information.

.fields() also deduplicates fields, so this is not reported, but I think it should:

@dataclass
class C:
	a: KW_ONLY
	a: KW_ONLY  # Same name

@AlexWaygood
Copy link
Member

.fields() also deduplicates fields, so this is not reported, but I think it should:

That doesn't cause a TypeError at runtime, so I think it should at least be a different rule if we want to complain about it. It feels more like a lint rule to me.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@abhijeetbodas2001 abhijeetbodas2001 left a comment

Choose a reason for hiding this comment

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

Thanks! For me, this diff is also helpful as a tutorial on how to add a new diagnostic :)

@abhijeetbodas2001
Copy link
Contributor

It feels more like a lint rule to me.

Another idea for a lint rule here could be to recommend using _ instead of an actual variable name for the KW_ONLY annotated field.

@carljm carljm merged commit 20d73dd into astral-sh:main Jun 20, 2025
36 checks passed
dcreager added a commit that referenced this pull request Jun 20, 2025
* main: (21 commits)
  [`flake8-logging`] Avoid false positive for `exc_info=True` outside `logger.exception` (`LOG014`) (#18737)
  [`flake8-pie`] Small docs fix to `PIE794` (#18829)
  [`pylint`] Ignore __init__.py files in (PLC0414) (#18400)
  Avoid generating diagnostics with per-file ignores (#18801)
  [`flake8-simplify`] Fix false negatives for shadowed bindings  (`SIM910`, `SIM911`) (#18794)
  [ty] Fix panics when pulling types for `ClassVar` or `Final` parameterized with >1 argument (#18824)
  [`pylint`] add fix safety section (`PLR1714`) (#18415)
  [Perflint] Small docs improvement to `PERF401` (#18786)
  [`pylint`] Avoid flattening nested `min`/`max` when outer call has single argument (`PLW3301`) (#16885)
  [`ruff`] Added `cls.__dict__.get('__annotations__')` check (`RUF063`) (#18233)
  [ty] Use `HashTable` in `PlaceTable` (#18819)
  docs: Correct collections-named-tuple example to use PascalCase assignment (#16884)
  [ty] ecosystem-analyzer workflow (#18719)
  [ty] Add support for `@staticmethod`s (#18809)
  unnecessary_dict_kwargs doc - a note on type checking benefits (#18666)
  [`flake8-pytest-style`] Mark autofix for `PT001` and `PT023` as unsafe if there's comments in the decorator (#18792)
  [ty] Surface matched overload diagnostic directly (#18452)
  [ty] Report when a dataclass contains more than one `KW_ONLY` field (#18731)
  [`flake8-pie`] Add fix safety section to `PIE794` (#18802)
  [`pycodestyle`] Add fix safety section to `W291` and `W293`  (#18800)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants