Fix False positive - Final local scope variable in Protocol

[GiorgosPapoutsakis]

This PR fixes and closes #17281 ,which reported a false positive when using Final within the local function scope of a protocol method.

With these changes:

• Local variables within protocol methods can be marked as Final.
• Protocol members still cannot be marked as Final

Modified semanal.py file and added a unit test in test-data/unit/check.final.test

[stroxler]

The tests seem good but I do have a question about whether the code change is implying new behavior for ClassVars.

[stroxler]

(at least as I'm reading it, this is the key change and hence my question about line 1623)

[stroxler]

It looks like this change is allowing classvars to be Final on protocols.

Is that intended? I don't see a test for it, and I'm also not entirely sure it makes sense... what would a Final protocol classvar even mean, that classes which implement it must have a final classvar of matching type?

[GiorgosPapoutsakis]

@stroxler Thank you for your feedback.
My intention was solely to fix the bug and didn’t intend to change any other behavior. Indeed upon reviewing, I agree that some of the changes I made were unnecessary. Probably there were not creating any issues with ClassVars as all the tests passed successfully( especially testFinalUsedWithClassVar ). I added one more unit test and removed the unnecessary code.

[GiorgosPapoutsakis]

I have squashed all of my commits into a single commit to simplify the commit history and make it easier to review.

[stroxler]

Looks good to me with the simplified change, tests look solid.

You'll still need a maintainer review, but approving because that may make it quicker (I volunteered to start helping with reviews but I'm still new to MyPy).###

[JelleZijlstra]

I feel this could theoretically still have bugs if there are nested classes involved (e.g., a class inside a protocol method). However, that's a much more obscure bug and I'm OK with merging this so we fix the reported bug.

[stroxler]

If it helps, I'd be happy to take a look at nested classes and either add tests proving this works or come up with a fix-forward as a follow-up.

[sterliakov]

@JelleZijlstra as far as I understand, this implementation doesn't suffer from such problems. In case of a nested class (or a class defined in a method) we do not work in Protocol class context, it's not the topmost context item.

I cannot find any explicit documentation entries regarding Final and final in inner classes of Protocols, but for me this behaviour totally makes sense. The following is green:

[case testFinalInProtocol]
from typing import Final, Protocol, final

class P(Protocol):
    var1 : Final[int] = 0 # E: Protocol member cannot be final

    @final # E: Protocol member cannot be final
    def meth1(self) -> None:
        var2: Final = 0

    def meth2(self) -> None:
        var3: Final = 0

    def meth3(self) -> None:
        class Inner:
            var3: Final = 0

            @final
            def inner(self) -> None: ...

    class Inner:
        var3: Final = 0

        @final
        def inner(self) -> None: ...

(it may be worth including this test to document the behaviour mypy sticks to)

For reference, pyright ignores this altogether apparently, allowing Protocols with final attributes and methods.

[JelleZijlstra]

Thanks! Would you mind adding a test for this behavior?

I missed that the implementation already looks only at the topmost level in the stack.

I don't really know what an inner class directly in a protocol should mean, but that's a separate issue.

[sterliakov]

Would be glad to, but it's not my fork, sorry, I just wanted to review something:)

@stroxler would you consider incorporating my suggested testcase?

[JelleZijlstra]

Ah sorry, should have checked the usernames. I can push your test case to this branch.

[stroxler]

Interesting that PyRight allows this.

Now that I think about it, there's arguably a use case for Final attributes in protocols (which is not to say I necessarily think MyPy has to support it).

Final in a class attribute actually means two mostly unrelated things, which I think is actually a bit problematic:

• final in the inheritance sense, i.e. not overridable
• not mutable

This actually strikes me as a gap in the typing semantics because it makes perfect sense to talk about non-final non-mutable attributes.

In the case of a Protocol, non-overrideability doesn't really make any sense, but non-mutability does.

Maybe I'll bring it up on the typing discourse at some point. In principle we could extend ReadOnly to be usable for a non-final but non-mutable attribute.

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

[JelleZijlstra]

You can also use an @property with no setter in a protocol to indicate a read-only attribute. I believe mypy allows any attribute if you do this, but pyright requires that you actually use a property.

Agree that expanding ReadOnly to protocols could be useful.