Skip to content

Support metaclasses - #2475

Merged
gvanrossum merged 18 commits into
python:masterfrom
elazarg:metaclasses
Feb 7, 2017
Merged

gvanrossum merged 18 commits into
python:masterfrom
elazarg:metaclasses

Conversation

@elazarg

@elazarg elazarg commented Nov 22, 2016 •

Copy link
Copy Markdown
Contributor

Reopening #2392 (itself a reopen of #2365).

Store the type of the metaclass in TypeInfo, and use it when possible, instead of builtins.type.

I assume metaclasses always inherit from type.

This PR will help fixing #2305 and #741, but the fix is not part of this PR.

@elazarg elazarg changed the title Support metaclasses [WIP] Support metaclasses Nov 22, 2016
@elazarg

elazarg commented Nov 22, 2016

Copy link
Copy Markdown
Contributor Author

(No tests yet)

@JukkaL

JukkaL commented Nov 25, 2016

Copy link
Copy Markdown
Collaborator

Looks reasonable -- thanks for working on this! One thing wasn't clear: does this follow the MRO in search of the metaclass?

Also, we may also want to check for incompatible metaclasses in the MRO, but that can be a separate issue and doesn't need to be covered here.

@JukkaL

JukkaL commented Nov 25, 2016

Copy link
Copy Markdown
Collaborator

(I only did a quick pass, not a full review.)

@gvanrossum

Copy link
Copy Markdown
Member

Hey @elazarg, are you still planning to work on this?

Comment thread test-data/unit/semanal-errors.test Outdated
class A(metaclass=abc.Foo): pass
[out]
main:2: error: Name 'abc.Foo' is not defined
main:2: error: Invalid metaclass 'abc.Foo'

@elazarg elazarg Dec 13, 2016 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Which one is preferable?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that abc.Foo does not exist, I prefer "Name 'abc.Foo' is not defined" -- if I were to see the other I would assume that there is an abc.Foo that was unacceptable as a metaclass. (Typos and large lists of imports might obscure that it's not defined at all.)

@elazarg

elazarg commented Dec 13, 2016 •

Copy link
Copy Markdown
Contributor Author

Yes, sorry. Todo:

  • Add test
  • Find out why would self.mro be None
  • Follow mro better

Also, I should look for other places where the fallback is needed, other than for iterable. (I must note that the tests for fallbackness feels awkward and error prone)

@elazarg elazarg changed the title [WIP] Support metaclasses Support metaclasses Dec 14, 2016
@elazarg

elazarg commented Dec 14, 2016

Copy link
Copy Markdown
Contributor Author

I think this is ready for a review.

def __iter__(self) -> Iterator[int]: yield 1

class Good(metaclass=GoodMeta): pass
for _ in Good: pass

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this works, shouldn't list(C) work too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just assumed it will work. Turns out it didn't, but after handling the fallback in the solver it does.

Comment thread mypy/nodes.py Outdated

def calculate_metaclass_type(self) -> 'Optional[mypy.types.Instance]':
if self.mro is None:
# XXX why does this happen?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you have a sample that triggers this condition?

Presumably the clue is the comment in __init__ on line 1940 above. Maybe it has something to do with the MRO being calculated twice -- the first time (in semanal.py on line 850) it may fail if there's an import cycle causing a base class being a forward reference; the second time on line 3128 in ThirdPass, called from build.py line 1440. (There's also a call from fixup.py, but I think that's not what you're seeing.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Turns out it happens during deserialization in incremental mode: the type of a direct base class may be None. (I should have looked into it long time ago. Sorry)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I will remove the call from deserialize, since it shouldn't be done yet - it should be done from fixup.py. Does it sound reasonable?

@gvanrossum

gvanrossum commented Feb 6, 2017 via email

Copy link
Copy Markdown
Member

Comment thread mypy/semanal.py
self.fail("Invalid metaclass '%s'" % defn.metaclass, defn)
return
inst = fill_typevars(sym.node)
assert isinstance(inst, Instance)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can trigger this assert as follows:

class M(Tuple[int]): pass
class C(metaclass=M): pass

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it should be an error then. Do you agree? I don't want to go silently to a fallback,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you can add the check to line 911.

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you also look into #1771? Possibly as a follow-up PR?

Comment thread mypy/nodes.py Outdated
candidates = {s.declared_metaclass for s in self.mro} - {None}
for c in candidates:
if all(other.type in c.type.mro for other in candidates):
return c

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How sure are you that the answer does not depend on the order of candidates?

@elazarg elazarg Feb 7, 2017 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not at all. Perhaps redundant tests checks are better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have replaced the set with a simple list

candidates = [s.declared_metaclass for s in self.mro if s.declared_metaclass is not None]

Comment thread mypy/semanal.py
self.fail("Invalid metaclass '%s'" % defn.metaclass, defn)
return
inst = fill_typevars(sym.node)
assert isinstance(inst, Instance)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you can add the check to line 911.

Comment thread test-data/unit/check-classes.test Outdated

class A(metaclass=M): pass

reveal_type(A.x) # E: Revealed type is 'builtins.int'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line seems redundant, it's the same as in the previous test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have removed it in the last commit but it still appears here.

@elazarg

elazarg commented Feb 7, 2017

Copy link
Copy Markdown
Contributor Author

I will try to look into #1771 later. But it feels like it will require a hack (or a very deep change).

@gvanrossum

gvanrossum commented Feb 7, 2017 via email

Copy link
Copy Markdown
Member

@gvanrossum
gvanrossum merged commit cab1e0d into python:master Feb 7, 2017
@gvanrossum

Copy link
Copy Markdown
Member

Thanks. Great work!

JukkaL pushed a commit that referenced this pull request Feb 7, 2017
Fix an issue in #2475 - metaclasses are not calculated for subclasses (this 
is needed for Enum).
JukkaL pushed a commit that referenced this pull request Feb 7, 2017
Fix #2818.

ABCMeta requires special treatment.

Related: #2475
JukkaL pushed a commit that referenced this pull request Feb 7, 2017
Fix subissue of #2413.

Follow-up for #2475

* check for member in fallback

* check for fallback only for type objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants