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

False positives when redefining property in subclass #5936

Open
mthuurne opened this issue Nov 21, 2018 · 5 comments
Open

False positives when redefining property in subclass #5936

mthuurne opened this issue Nov 21, 2018 · 5 comments
Labels
false-positive mypy gave an error on correct code feature priority-1-normal topic-descriptors Properties, class vs. instance attributes

Comments

@mthuurne
Copy link
Contributor

I'm running mypy on the following code: (no additional arguments)

class Base:
	def __init__(self, value):
		self._value = value

	@property
	def value(self):
		return self._value

class Sub(Base):
	@Base.value.setter
	def value(self, value):
		self._value = value

And it reports:

inherited_property.py:10: error: "Callable[[Any], Any]" has no attribute "setter"

Line 10 is @Base.value.setter, so it seems to have a problem with referring to a property defined in a different class. Python itself has no problem with this though.

If the property has a setter in the base class, a slightly different error is reported:

class Base:
	def __init__(self, value):
		self._value = value

	@property
	def value(self):
		return self._value

	@value.setter
	def value(self, value):
		self._value = value

class Sub(Base):
	@Base.value.setter
	def value(self, value):
		self._value = value

This results in mypy reporting:

inherited_property.py:14: error: overloaded function has no attribute "setter"

Again the error is reported on the @Base.value.setter line.

I'm using a mypy dev snapshot (0.650+) on Python 3.6.5.

I found the existing issue #220, but that is so generic that I figured it would make sense to open a new separate issue for tracking this specific use case.

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 22, 2018

Yeah, this is something that mypy should perhaps support. This is the first time I've seen this idiom, so it's probably not very common though.

This workaround works currently:

class Base:
	def __init__(self, value: int) -> None:
		self._value = value

	@property
	def value(self) -> int:
		return self._value

class Sub(Base):
	@property
	def value(self) -> int:
		return super().value

	@value.setter
	def value(self, value: int) -> None:
		self._value = value

@JukkaL JukkaL added feature priority-1-normal false-positive mypy gave an error on correct code labels Nov 22, 2018
@mthuurne
Copy link
Contributor Author

This problem might be a special case of #4644.

@mthuurne
Copy link
Contributor Author

Actually it looks like both are a special case of #1465, as pointed out in this comment.

Changing @value.setter to @Base.value.setter in your example is accepted by mypy. So it seems the problem is not the Base.value idiom, but the fact that mypy doesn't recognize a property setter unless it is directly preceded by the getter.

@dmanser92
Copy link

I had the same issue. Because I have a lot of sub-classes and I don't want to copy the property around so many times I ended up doing this:

  • added a type-ignore comment to the decorators where I overwrite the setters, like @Base.value.setter # type: ignore[attr-defined]
  • made a comment in the base-class explaining why I do this and pointing to this post

@AlexWaygood AlexWaygood added the topic-descriptors Properties, class vs. instance attributes label Apr 1, 2022
Rocamonde added a commit to HumanCompatibleAI/imitation that referenced this issue Sep 7, 2022
Rocamonde added a commit to HumanCompatibleAI/imitation that referenced this issue Oct 8, 2022
#534)

* Initial mypy configuration

* Fix types: test_envs.py

* Fix types: conftest.py

* Fix types: tests/util

* Fix types: tests/scripts

* Fix types: tests/rewards

* Fix types: tests/policies

* Incorrect decorator in update_stats method form networks.py::BaseNorm

* Fix types: tests/algorithms (adersarial and bc)

* Fix types: tests/algorithms (dagger and pc)

* Fix types: tests/data

* Linting

* Linting

* Fix types: algorithms/preference_comparisons.py

* Fix types: algorithms/mce_irl.py

* Formatting, fixed minor bug

* Clarify why types are ignored

* Started fixing types on algorithms/density.py

* Linting

* Linting (add back type ignore after reformatting)

* Fixed types: imitation/data/types.py

* Fixed types (started): imitation/data/

* Fixed types: imitation/data/buffer.py

* Fixed bug in buffer.py

* Fixed types: imitation/data/rollout.py

* Fixed types: imitation/data/wrappers.py

* Improve makefile to support automatic cache cleaning

* Fixed types: imitation/testing/

* Linting, fixed wrong return type in rewards.predict_processed_all

* Fixed types: imitation/policies/

* Formatting

* Fixed types: imitation/rewards/

* Fixed types: imitation/rewards/

* Fixed types: imitation/scripts/

* Fixed types: imitation/util/ and formatting

* Linting and formatting

* Bug fixes for test errors

* Linting and typing

* Improve typing in algorithms

* Formatting

* Bug fix

* Formatting

* Fixes suggested by Adam.

* Fix mypy version.

* Update TabularPolicy.predict to match base class

* Fix not checking for dones

* Change for loop to dict comprehension

* Remove is_ensemble to clear up type checking errors

* Reduce code duplication and general cleanup

* Fix type annotation of step_dict

* Change List to Sequence

* Fix density.py::DensityAlgorithm._set_demo_from_batch

* Fixed n_steps (OnPolicyAlgorithm)

* Fix errors in tests

* Include some suggestions into rollout.py and preference_comparisons.py

* Formatting

* Fix setter error as per python/mypy#5936

* add reason for assertion.

* Fix style guide violation: https://google.github.io/styleguide/pyguide.html#22-imports

* Update src/imitation/scripts/parallel.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Move kwargs to the end.

* Swap order of expert_policy_type and expert_policy_path validation check

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Update tests/rewards/test_reward_fn.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Explicit random state setting and fix corresponding tests (except notebooks, sacred config, scripts)

* Fix notebooks; add script to clean notebooks

* Fix all tests.

* Formattting.

* Additional fixes

* Linting

* Remove automatically generated `_api` docs files too on `make clean`

* Fix docstrings.

* Fix issue with next(iter(iterable))

* Formatting

* Remove whitespace

* Add TODO message to remove type ignore later

* Remove unnecessary assertion.

* Fixed types in density.py set_demonstrations

* Added type ignore to pytype bug

* Fix_get_first_iter_element and add tests

* Bugfix in BC and tests -- masked as previously iterator ran out too early!

* Remove makefile for now

* Added link to SB3 issue for future reference.

* Fix types of train_imitation
Only return "expert_stats" if all trajectories have reward.

* Modify assert in test_bc to reflect correct type

* Add ci/clean_notebooks.py to CI checks

* Improve clean_notebooks.py by allowing checking only mode.

* Add ipynb notebook checks to CI

* Add support for explicit files for notebook cleaning

* Clean notebooks

* Small improvements in util.py

* Replace TransitionKind with TransitionsMinimal

* Delete unused statement in test

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Make type ignore specific to pytype

* Linting

* Migrate from RandomState (deprecated) to Generator

* Add backticks to error message

* Create "AnyNorm" alias

* Small fix

* Add additional checks to shapes in _set_demo_from_batch

* Fix RolloutStatsComputer type

* Improved logging/messages in clean_notebooks.py

* Fix issues resulting from merge

* Bug fix

* Bug fix (wasn't really fixed before)

* Fixed docs example of BC

* Fix bugs resulting from merge

* Fix docs (dagger.rst) caught by sphinx CI

* Add mypy to CI

* Continue fixing miscellaneous type errors

* Linting

* Fix issue with normalize_input_layer type

* Add support for checking presence of generic type ignores

* Allow subdirectories in notebook clean

* Add full typing support for TransitionsMinimal as a sequence

* Fix types for density.py

* Misc fixes

* Add support for prefix context manager in logger (from #529)

* Added back accidentally removed code

* Replaced preference comparisons prefix with ctx manager

* Fixed errors

* Bug fixes

* Docstring fixes

* Fix bug in serialize.py

* Fixed codecheck by pointing notebook checks to docs

* Add rng to mce_irl.rst (doctest)

* Add rng to density.rst (doctest)

* Fix remaining rst files

* Increase sample size to reduce flakiness

* Ignore files not passing mypy for now

* Comment in wrong line

* Comment in wrong line

* Move excluded files to argument

* Add quotes to mypy arg call

* Fix CI mypy call

* Fix CI yaml

* Break ignored files up into one line each

* Address PR comments

* Point SB3 to master to include bug fix

* Do not follow imports for ignored files

* Format / fix tests for context manager

* Switch to sb3 1.6.1

* Formatting

* Remove unused import

* Remove unused fixture

* Add coveragerc file

* Add utils test

* Add tests and asserts

* Add test to synthetic gatherer

* Add trajectory unwrap tests

* Formatting

* Remove bracket typo

* Fix .coveragerc instruction

* Improve density algo coverage and bug fixes

* Fix bug in test

* Add pragma no cover updates

* Minor coverage tweaks

* Fix iterator test

* Update ci/check_typeignore.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* DRY clean_notebooks.py

* Minor tweak in check_typeignore.py

* Added all checks to CI

* Move imports to top-level

* Move main to main() function in script

* Minor fixes

* Remove tweak after new SB3 release

* Split main() into helper functions.

* Fix edge case of n=0 in seed generator

* Update src/imitation/scripts/common/rl.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Fix general type ignore in src

* Fix type ignore errors

* Formatting

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Update src/imitation/scripts/common/rl.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Replace todo with todo+issue link

* Add explicit type ignore arg

* Add excluded files to code_checks.sh

* Unbreak line

* Misc fixes

* Add training to the density algorithm

* Fix no attribute error

* Type ignore to `with raises` test

* Remove unused import

* Check typeignore for all SRC files

* Clean notebooks

* Remove unused import

* Ignore the file itself from typeignore check

* Add exception to docstring

* Fix bad naming for clean_notebooks

Co-authored-by: Adam Gleave <adam@gleave.me>
Rocamonde added a commit to HumanCompatibleAI/imitation that referenced this issue Oct 11, 2022
* Initial mypy configuration

* Initial change to get the PR up

* Initial review at replacing os.path

* Bug fixes from tests

* Fix types: test_envs.py

* Fix types: conftest.py

* Fix types: tests/util

* Fix types: tests/scripts

* Fix types: tests/rewards

* Fix types: tests/policies

* Incorrect decorator in update_stats method form networks.py::BaseNorm

* Fix types: tests/algorithms (adersarial and bc)

* Fix types: tests/algorithms (dagger and pc)

* Fix types: tests/data

* Linting

* Linting

* Fix types: algorithms/preference_comparisons.py

* Fix types: algorithms/mce_irl.py

* Formatting, fixed minor bug

* Clarify why types are ignored

* Started fixing types on algorithms/density.py

* Linting

* Linting (add back type ignore after reformatting)

* Fixed types: imitation/data/types.py

* Fixed types (started): imitation/data/

* Fixed types: imitation/data/buffer.py

* Fixed bug in buffer.py

* Fixed types: imitation/data/rollout.py

* Fixed types: imitation/data/wrappers.py

* Improve makefile to support automatic cache cleaning

* Fixed types: imitation/testing/

* Linting, fixed wrong return type in rewards.predict_processed_all

* Fixed types: imitation/policies/

* Formatting

* Fixed types: imitation/rewards/

* Fixed types: imitation/rewards/

* Fixed types: imitation/scripts/

* Fixed types: imitation/util/ and formatting

* Linting and formatting

* Bug fixes for test errors

* Linting and typing

* Improve typing in algorithms

* Formatting

* Bug fix

* Formatting

* Fixes suggested by Adam.

* Fix mypy version.

* Fix bugs

* Remove unused imports

* Formatting

* Added parse_path func and refactored code to use it

* Fix typing, linting

* Update TabularPolicy.predict to match base class

* Fix not checking for dones

* Change for loop to dict comprehension

* Remove is_ensemble to clear up type checking errors

* Reduce code duplication and general cleanup

* Fix type annotation of step_dict

* Change List to Sequence

* Fix density.py::DensityAlgorithm._set_demo_from_batch

* Fixed n_steps (OnPolicyAlgorithm)

* Fix errors in tests

* Include some suggestions into rollout.py and preference_comparisons.py

* Formatting

* Fix setter error as per python/mypy#5936

* add reason for assertion.

* Fix style guide violation: https://google.github.io/styleguide/pyguide.html#22-imports

* Update src/imitation/scripts/parallel.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Move kwargs to the end.

* Swap order of expert_policy_type and expert_policy_path validation check

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Update tests/rewards/test_reward_fn.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Explicit random state setting and fix corresponding tests (except notebooks, sacred config, scripts)

* Fix notebooks; add script to clean notebooks

* Fix all tests.

* Formattting.

* Additional fixes

* Linting

* Remove automatically generated `_api` docs files too on `make clean`

* Fix docstrings.

* Fix issue with next(iter(iterable))

* Formatting

* Remove whitespace

* Add TODO message to remove type ignore later

* Remove unnecessary assertion.

* Fixed types in density.py set_demonstrations

* Added type ignore to pytype bug

* Fix_get_first_iter_element and add tests

* Bugfix in BC and tests -- masked as previously iterator ran out too early!

* Remove makefile for now

* Added link to SB3 issue for future reference.

* Fix types of train_imitation
Only return "expert_stats" if all trajectories have reward.

* Modify assert in test_bc to reflect correct type

* Add ci/clean_notebooks.py to CI checks

* Improve clean_notebooks.py by allowing checking only mode.

* Add ipynb notebook checks to CI

* Add support for explicit files for notebook cleaning

* Clean notebooks

* Small improvements in util.py

* Replace TransitionKind with TransitionsMinimal

* Delete unused statement in test

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Update src/imitation/util/util.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Make type ignore specific to pytype

* Linting

* Migrate from RandomState (deprecated) to Generator

* Add backticks to error message

* Create "AnyNorm" alias

* Small fix

* Add additional checks to shapes in _set_demo_from_batch

* Fix RolloutStatsComputer type

* Improved logging/messages in clean_notebooks.py

* Fix issues resulting from merge

* Bug fix

* Bug fix (wasn't really fixed before)

* Fixed docs example of BC

* Fix bugs resulting from merge

* Fix docs (dagger.rst) caught by sphinx CI

* Add mypy to CI

* Continue fixing miscellaneous type errors

* Linting

* Fix issue with normalize_input_layer type

* Add support for checking presence of generic type ignores

* Allow subdirectories in notebook clean

* Add full typing support for TransitionsMinimal as a sequence

* Fix types for density.py

* Misc fixes

* Add support for prefix context manager in logger (from #529)

* Added back accidentally removed code

* Replaced preference comparisons prefix with ctx manager

* Fixed errors

* Bug fixes

* Docstring fixes

* Fix bug in serialize.py

* Fixed codecheck by pointing notebook checks to docs

* Add rng to mce_irl.rst (doctest)

* Add rng to density.rst (doctest)

* Fix remaining rst files

* Increase sample size to reduce flakiness

* Ignore files not passing mypy for now

* Comment in wrong line

* Comment in wrong line

* Move excluded files to argument

* Add quotes to mypy arg call

* Fix CI mypy call

* Fix CI yaml

* Break ignored files up into one line each

* Address PR comments

* Point SB3 to master to include bug fix

* Small bug fixes

* Small bug fixes

* Sort import

* Linting

* Do not follow imports for ignored files

* Fix tests for context managers

* Format / fix tests for context manager

* Switch to sb3 1.6.1

* Formatting

* Upgrade Python version in Windows CI

* Remove unused import

* Remove unused fixture

* Add coveragerc file

* Add utils test

* Add tests and asserts

* Add test to synthetic gatherer

* Add trajectory unwrap tests

* Formatting

* Remove bracket typo

* Fix .coveragerc instruction

* Improve density algo coverage and bug fixes

* Fix bug in test

* Add pragma no cover updates

* Minor coverage tweaks

* Fix iterator test

* Add test for parse_path

* Updates on sacred util

* Mark type ignore rule

* Mark type ignore rule

* Miscellaneous bug fixes and improvements

* Reformat hanging line

* Ignore parse path checks for windows

* Add trailing comma

* Minor changes

* No newline end of file

* Update src/imitation/data/types.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Update src/imitation/data/types.py

Co-authored-by: Adam Gleave <adam@gleave.me>

* Include suggestions from Adam

Co-authored-by: Adam Gleave <adam@gleave.me>
@Cleptomania
Copy link

I've run in to this issue as well and created #14684, and I've found another work-around that produces no errors from MyPy. It seems to not be problematic if the property in the base class is declared using the property() function as opposed to the decorators, like below, note that my specific use case was attempting to only override the setter of the Base class.

class Base:
    def __init__(self, value: int) -> None:
        self._value = value

    def _get_value(self) -> int:
        return self._value

    def _set_value(self, value: int):
        self._value = value

    value = property(_get_value, _set_value)
    
class Sub(Base):
    def __init__(self, value: int) -> None:
        super().__init__(value)
        
    @Base.value.setter
    def value(self, value: int) -> None:
        Base.value.fset(self, value)
        print("Do Something after the Base setter")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false-positive mypy gave an error on correct code feature priority-1-normal topic-descriptors Properties, class vs. instance attributes
Projects
None yet
Development

No branches or pull requests

5 participants