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

Final names and attributes #5522

Merged
merged 58 commits into from Sep 11, 2018

Conversation

Projects
None yet
6 participants
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Aug 22, 2018

Fixes #1214
Fixes python/typing#286
Fixes python/typing#242 (partially, other part is out of scope)

This is a working implementation of final access qualifier briefly discussed at PyCon typing meeting. Final names/attributes can be used to have more static guarantees about semantics of some code and can be used by other tools like mypyc for optimizations.

We can play with this implementation before starting to write an actual PEP.

The basic idea is simple: once declared as final, a name/attribute can't be re-assigned, overridden, or redefined in any other way. For example:

from typing import Final

NO: Final = 0
YES: Final = 255

class BaseEngine:
    RATE: Final[float] = 3000

YES = 1  # Error!

class Engine(BaseEngine):
    RATE = 9000  # Also an error!

For more use cases, examples, and specification, see the docs patch.

Here are some comments on decisions made:

  • What can be final? It is hard to say what semantic nodes are important, I started from just module and class constants, but quickly realized it is hard to draw the line without missing some use cases (in particular for mypyc). So I went ahead and implemented all of them, everything can be final: module constants, class-level and instance-level attributes, method, and also classes.
  • Two names or one name? I currently use two names Final for assignments and @final for decorators. My PEP8-formatted mind just can't accept @Final :-)
  • Should re-exported names keep they const-ness? I think yes, this is a very common pattern, so it looks like this is a sane default.
  • What to do with instance-level vs class-level attributes? The point here is that mypy has a common namespace for class attributes. I didn't want to complicate things (including the mental model), so I just decided that one can't have, e.g., a name that is constant on class but assignable on instances, etc. Such use cases are relatively rare, and we can implement this later if there will be high demand for this.

...deferred features:

  • I didn't implement any constant propagation in mypy yet. This can be done later on per use-case
    basis. For example:
    fields: Final = [('x', int), ('y', int)]
    NT = NamedTuple('NT', fields)
  • Should final classes be like sealed in Scala? I think probably no. On one hand it could be be a nice feature, on other hand it complicates the mental model and is less useful for things like mypyc.
  • I don't allow Final in function argument types. One argument is simplicity, another is I didn't see many bugs related to shadowing an argument in function bodies, finally people might have quite different expectations for this. If people will ask, this would be easy to implement.

...and implementation internals:

  • There are two additional safety nets that I don't mention in the docs: (a) there can be no TypeVars in the type of class-level constant, (b) instance-level constant can't be accessed on the class object.
  • I generate errors for re-definitions in all subclasses, not only in immediate children. I think this is what most people would want: turning something into a constant will flag most re-assignment points.
  • It is hard to always get a single error sometimes (I have two error messages, one for re-assignment, another for overriding) because of how mypy creates new Vars. But I think having two errors occasionally is fine.
  • We store the final_value for constants initialized with a simple literal, but we never use it. This exists only for tools like mypyc that may use it for optimizations.

The PR is open for questions, suggestions, and comments.

cc @ambv @rchen152 @vlasovskikh

@ilevkivskyi ilevkivskyi requested a review from msullivan Aug 22, 2018

@mvaled

This comment has been minimized.

Copy link

mvaled commented Aug 22, 2018

@med-merchise you were asking for this, I think.

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator

Michael0x2a commented Aug 22, 2018

This is super cool!

Just two comments -- first, it might be nice if there were some tests + docs about Final and control flow. For example, the following program typechecks without an error:

[case testFinalControlFlow]
from typing import Final

for val in [1, 2, 3]:
    x: Final = val

[builtins fixtures/list.pyi]

I found this a bit surprising since we are rebinding x several times/tools like mypyc can no longer assume that 'x' will have the same value within the loop. But then again, maybe this is ok? I think i'd be sort of hard to accidentally do something weird here, and any weirdness that does happen will be localized since x will be actually final after the loop is over...

Not sure, but either way, some clarification might be nice (unless you were planning on doing this in a different PR).

Second, I expect that this proposal will be rejected immediately for being too unpythonic + too messy to implemented, but I figure I might as well get it out of the way... How do we feel about adding a flag to mypy that makes all variables and attributes Final by default?

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Aug 23, 2018

@Michael0x2a

first, it might be nice if there were some tests + docs about Final and control flow

I would actually prohibit Final in loops. Moreover, I would propose to also prohibit Final in if statements, unless the truthiness of condition is known statically (like sys.version > 3.6).

In addition, I just noticed that I forgot about in-place assignments, will fix this soon.

Second, I expect that this proposal will be rejected immediately for being too unpythonic

I am not sure I understand this comment. Rejected immediately by whom? There were no objections during PyCon typing meeting, moreover many people liked the idea.

too messy to implemented

Same story here, sorry :-) Do you want to say that the implementation in this PR is messy?

How do we feel about adding a flag to mypy that makes all variables and attributes Final by default?

I am not sure there is any existing Python program larger than 1k lines that will type-check with such flag. Plus we already have dozens of flags. So I am against this unless people will ask often for this.

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator

Michael0x2a commented Aug 23, 2018

I am not sure I understand this comment. Rejected immediately by whom? There were no objections during PyCon typing meeting, moreover many people liked the idea.

Ah, I meant to say that I expected my proposal to add a flag to make final always-on by default would be rejected immediately by you.

(Which is what just happened -- but it was worth a shot 😄)

@JukkaL
Copy link
Collaborator

JukkaL left a comment

Thanks for all the updates! Here's another review pass. I still haven't reviewed test case changes and haven't tried to use the new features.

@@ -0,0 +1,340 @@
Type qualifiers

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

I'm not sure if these are actually type qualifiers, at least using the existing definition for the term. For example, looking at https://en.wikipedia.org/wiki/Type_qualifier, the article explicitly mentions that Java final is not a type qualifier, and Final is more similar to Java final than, say, the C const qualifier, which is a type qualifier. If Final was a type qualifier, I'd expect that List[Final[int]] would be valid.

A slightly better name might be "Name qualifiers", but it's not an established term. Using it as a section header would be confusing, as almost nobody reading would know what it means until they've read the introduction.

Not sure what's the best way to structure this. Maybe ClassVar could be described in class basics, as it's a pretty simple feature. This section would then only talk about final, and the section title could be something like "Final names, methods and classes". The motivation is that "final" is not a single feature, but arguably three related features: the Final modifier for variables, the final decorator for methods and the final decorator for classes.

****************************

By default mypy assumes that a variable declared in the class body is
an instance variable. One can mark names intended to be used as class variables

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

This is a bit confusing, and arguably not quite true. Mypy assumes that that it can used like either a class or an instance variable. And whether an attribute is defined in a method or in the class body doesn't make a difference -- they can both be used as class or instance variables (I'd like to change this, however). We could describe ClassVar as "... mark names only intended to be used as class variables ...".

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 6, 2018

Author Collaborator

This is a bit confusing, and arguably not quite true. Mypy assumes that that it can used like either a class or an instance variable.

FWIW, this is the language used in PEP 526. When someone writes:

class C:
    x: int

C.x = 0

variable x is an instance variable, and its default can be set on class.


By default mypy assumes that a variable declared in the class body is
an instance variable. One can mark names intended to be used as class variables
with a special type qualifier ``typing.ClassVar``. For example:

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

Nits: ClassVar is not a type qualifier based on established conventions. I'd leave out "special" out as it doesn't add anything to the discussion, and can scare some users away.

class Base:
attr: int # This is an instance variable
num_subclasses: ClassVar[int] # This is a class variable

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

Can you come with a more typical (and complete) example? Usually a class variable is initialized in the class body, and initializing it outside the class doesn't feel idiomatic. Also, it would be better to both initialize the class variable and update it. The current example only initializes it.

Also, from the name num_subclasses I expect some deep metaclass magic, which we should avoid in a basic example. I.e. maybe do something super concrete and nothing "meta" or abstract, to avoid the risk of confusing some readers.


Assigning a value to a variable in the class body doesn't make it a class
variable, it just sets a default value for an instance variable, *only*
names explicitly declared with ``ClassVar`` are class variables.

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

I just noticed that x: ClassVar = 1 results in type Any to be inferred for x, which is inconsistent with how Final works. I much prefer that way Final works (int would be inferred). What do you think about this?

def _reset_var_final_flags(self, v: Var) -> None:
v.is_final = False
v.final_unset_in_class = False
v.final_set_in_init = False

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

What about the final_value attribute?

@@ -187,6 +188,14 @@ def parse_test_cases(parent: 'DataSuiteCollector', suite: 'DataSuite',
path, p[i0].line))


MYPY = False
if MYPY:
if sys.version_info >= (3, 5):

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

This change seems unrelated?

# If constant value is a simple literal,
# store the literal value (unboxed) for the benefit of
# tools like mypyc.
self.final_value = None # type: Optional[Union[int, float, bool, str]]

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

This doesn't seem to get serialized?

@@ -55,4 +56,6 @@ class Mapping(Generic[T_contra, T_co]):

def runtime(cls: type) -> type: pass

def final(meth: T) -> T: pass

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

Add note mentioning that this is an unofficial extension.

@@ -16,6 +16,7 @@ NamedTuple = 0
Type = 0
no_type_check = 0
ClassVar = 0
Final = 0

This comment has been minimized.

@JukkaL

JukkaL Sep 3, 2018

Collaborator

After one mypy release cycle fix bugs and design errors and add Final to typing_extensions. It would be good to have a draft PEP ready at this point.

We can't really advertise a feature in mypy documentation if it doesn't work yet (say, if Final and final are missing from typing_extensions). So looks like we need to do one of these:

  1. Update typing_extensions before the first mypy release that includes this PR. We should probably mention which version of typing_extensions is required to use Final/final in mypy docs.
  2. Add the new definitions to mypy_extensions and release the changes before the first mypy release that includes this PR. Update documentation to use mypy_extensions in all relevant examples, instead of typing_extensions.
  3. Postpone merging this PR until we've done one of the above two things.

Also, please add a note here mentioning that this is still an unofficial extension.

I'm not sure if it's important to start using Final in stubs immediately. The benefit is pretty minor compared to protocols, as this is primarily about fixing false negatives, and the concept of 'final' doesn't really exist in Python as such, so deciding which things should be final is less obvious. Duck typing, on the other hand, has a long history in Python and is pretty essential for certain APIs.

ilevkivskyi added some commits Sep 3, 2018

@JukkaL
Copy link
Collaborator

JukkaL left a comment

Did another pass at the documentation. Looks much better now!

I think that it's still a bit too verbose and would be better if some things were left out or shortened (left a bunch of suggestions). Since this is not a specification/PEP, we can leave out special cases that should be obvious based on errors generated by mypy. Other documentation sections don't go into as much detail and it would be good retain a consistent tone and level of detail across the entire documentation.

Final names, methods and classes
================================

You can declare a variable or attribute as final, which means that the variable

This comment has been minimized.

@JukkaL

JukkaL Sep 7, 2018

Collaborator

In think that the section structure could be improved by doing it like this:

= Final names, methods, and classes

<short explanation of final names (variables/attributes), methods and classes, ~one paragraph>

== Final names

<all discussion specific to Final>

== Final methods

<all discussion specific to @final when used with methods>

== Final classes

<all discussion specific to @final when used with classes>

import uuid
from typing_extensions import Final
class Snowflake:

This comment has been minimized.

@JukkaL

JukkaL Sep 7, 2018

Collaborator

This example feels a bit too clever. I'd prefer something super straightforward that doesn't have these issues:

  • Snowflake would be an awkward name in real code. I think it's better to use more obviously dummy names such as Foo or C instead, but it would be even better to have somewhat realistic names.
  • The subclass doesn't actually override id properly since __init__ is inherited. This is kind of confusing -- only the attribute default would be overridden.

Here's a suggested simpler example:

class Window:
    BORDER_WIDTH: Final = 2
    ...

class ListView(Window):
    BORDER_WIDTH = 3  # Error: can't override a final attribute
    ...
Base.DEFAULT_ID = 1 # Error: can't override a final attribute
Another use case for final attributes is where a user wants to protect certain
instance attributes from overriding in a subclass:

This comment has been minimized.

@JukkaL

JukkaL Sep 7, 2018

Collaborator

Nit: this can also be used for class attributes. Maybe just leave out "instance"?

RATE = 300 # Error: can't assign to final attribute
Base.DEFAULT_ID = 1 # Error: can't override a final attribute
Another use case for final attributes is where a user wants to protect certain

This comment has been minimized.

@JukkaL

JukkaL Sep 7, 2018

Collaborator

Style nit: We don't use "a user". Prefer "you" instead, but in this case it can just be left out. For example: "Another use case for final attributes is to protect certain attributes form being overridden in a subclass".

Show resolved Hide resolved docs/source/final_attrs.rst Outdated
Show resolved Hide resolved docs/source/final_attrs.rst Outdated
Show resolved Hide resolved docs/source/final_attrs.rst Outdated
Show resolved Hide resolved docs/source/final_attrs.rst Outdated
Show resolved Hide resolved docs/source/final_attrs.rst Outdated
Show resolved Hide resolved docs/source/final_attrs.rst Outdated
@JukkaL
Copy link
Collaborator

JukkaL left a comment

Thanks for writing so many test cases! This is a solid set of test cases. I only came up with a few additional scenarios.

This is almost concludes my review. I'll do another light pass next week (I don't expect anything major).

self.check_final(s)
if (s.is_final_def and s.type and not has_no_typevars(s.type)
and self.scope.active_class() is not None):
self.fail("Constant declared in class body can't depend on type variables", s)

This comment has been minimized.

@JukkaL

JukkaL Sep 7, 2018

Collaborator

I think it would be better to not call final attributes constants. I.e. rename message to "Final attribute defined in ...".

Show resolved Hide resolved mypy/checkmember.py Outdated
Show resolved Hide resolved mypy/messages.py Outdated
Show resolved Hide resolved mypy/messages.py Outdated
Show resolved Hide resolved mypy/semanal.py Outdated
Show resolved Hide resolved test-data/unit/check-final.test
Show resolved Hide resolved test-data/unit/check-final.test Outdated
Show resolved Hide resolved test-data/unit/check-final.test
Show resolved Hide resolved test-data/unit/check-final.test
Show resolved Hide resolved test-data/unit/check-final.test Outdated
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Sep 11, 2018

@JukkaL I implemented only some comments in the docs. I will kill it in the next commit, then you or any one else can finish it whenever.

@JukkaL

JukkaL approved these changes Sep 11, 2018

Copy link
Collaborator

JukkaL left a comment

Thanks for the updates! Looks good now, just one minor thing I noticed.

base: TypeInfo, base_node: Optional[Node]) -> bool:
"""Check if an assignment overrides a final attribute in a base class.
This only check situations where either a node in base class is not a variable

This comment has been minimized.

@JukkaL

JukkaL Sep 11, 2018

Collaborator

Grammar: check -> checks

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Sep 11, 2018

OK, all tests passed, we are good to go. Thanks for review!

@ilevkivskyi ilevkivskyi merged commit 62e6f51 into python:master Sep 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ilevkivskyi added a commit to python/typing that referenced this pull request Sep 13, 2018

Add Final to typing_extensions (#583)
This is a runtime counterpart of an experimental feature added to mypy in python/mypy#5522

This implementation just mimics the behaviour of `ClassVar` on all Python/`typing` versions, which is probably the most reasonable thing to do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment