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

Make strictness of reportIncompatibleVariableOverride configurable #7703

Closed
black-sliver opened this issue Apr 14, 2024 · 3 comments
Closed
Labels
enhancement request New feature or request

Comments

@black-sliver
Copy link

Is your feature request related to a problem? Please describe.

We are trying to get more type annotations and type checking into a project that relies heavily on inheritance, where the type of members of the derived class are known, but are "incompatible overrides" of the base because they are appear mutable. Something like #5933 and #5998.
I'm well aware that this is a limitation of the current typing system, however it makes it impossible to have a strict-ish type checking without a ton of # type: ignore, or disabling reportIncompatibleVariableOverride, which could hide other errors and defeats the idea for us having stricter type checking in the first place.

Describe the solution you’d like

Until the problem is solved upstream, say with ReadOnly[SomeType], it would be amazing to have a way to disable the specific change / feature of reportIncompatibleVariableOverride to ignore the mutability aspect via configuration.

@black-sliver black-sliver added the enhancement request New feature or request label Apr 14, 2024
@erictraut
Copy link
Collaborator

There is no precedent for making specific diagnostic checks more or less strict. That would complicate the configuration model in pyright, creating yet another dimension of configurability. The configuration model in pyright is already complex, so that's not something I would entertain.

The way we'd normally handle a case like this is to split the diagnostic rule into two or more finer-grained diagnostic rules. The bar for doing this is relatively high, however, because doing this represents a breaking change of sorts. It means that any pyright user who has specified the diagnostic severity of reportIncompatibleVariableOverride will now need to reconfigure their settings to apply that diagnostic severity to multiple diagnostic rules. For this reason, splitting existing diagnostic rules is done only when there's a very clear need — as evidenced by strong signal from pyright users.

Since this is the first time someone has requested this particular feature, the signal is not currently strong enough for me to justify doing this.

I'm going to reject this enhancement request for now, but it's something I'm willing to revisit in the future if there is stronger signal from pyright users (e.g. in the form of upvotes on this issue).

I think the preferred way to address this issue is to amend the type system to expand the applicability of ReadOnly, as you suggested. Now that PEP 705 has been officially accepted and ReadOnly has been added to the type system, it should be relatively easy to advocate for the use of this special form in more places. If you're interested in championing the broader use of ReadOnly in the type system and would like to draft a specification for such a feature, I recommend starting a new discussion in the typing forum.

@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Apr 14, 2024
@black-sliver
Copy link
Author

Thanks for the quick reply!

I understand that the preferred way to address our issue is to enhance the type system to a point where we can specify what we want. The thing is that the change in #5955 (I think it's that one) made it so that the our existing type annotations are now invalid and there is no way to fix them.

My personal take on this topic is that as long as there is no way to specify what we want, the code snippet in #5933 should ignore if it's supposedly mutable or not, because pyright can't know if it is or not if there is no way to specify the difference. So to me #5955 made a breaking change. The reason I asked for it to be configurable is because I see the merit in supporting what #5933 wants, which is labeled as enhancement not as bugfix.

@erictraut
Copy link
Collaborator

I understand and appreciate that you're feeing some short-term pain, but there is a cost (both short term and long term) in adding temporary hacks to work around shortcomings in the type system.

In this case, the expanded use of ReadOnly is pretty straightforward to implement in a type checker. It would be about the same amount of work as implementing the configuration change that you've proposed here. The only piece that requires a bit more work for ReadOnly is writing the specification and gaining consensus from the typing community around that specification. If you're willing to champion and drive that discussion and spec, I'm happy to support you.

ReadOnly is already present in the typing module in Python 3.13 and back-ported to typing-extensions for all previous versions. It wouldn't require any changes to the runtime, and I don't think it would need a formal PEP (although the other four Typing Council members would have to sign off on that assessment). That means it should be relatively easy to get this through the spec process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants