Remove @builtinclass

[elazarg]

Following #599

[ddfisher]

Maybe @JukkaL can chime in here, but it seems to me that inheriting from object isn't the same as being a builtin class?

[elazarg]

@ddfisher you mean "being object isn't the same as being a builtin class"? I agree. It can be replaced with a lookup in a set. But there should be a test demonstrating this need.

[ddfisher]

Ah, yep -- I misread that. It looks to me like we don't currently use the @builtinclass functionality anywhere at all, so we should either make it work or just kill it completely.

[elazarg]

I guess either can be done in a separate PR.

[gvanrossum]

The nearest_builtin_ancestor() is actually used in a check for instance layout conflicts. This is currently a no-op because only object is considered builtin, but it is meant to guard against e.g. M.I. from list and dict simultaneously. So please at least put a TODO in that function explaining that it should be expanded, not deleted.

[elazarg]

str and int have conflicting definition of __add__, and this test is not about that.

[elazarg]

Added check for "builtin.lower-case" since it covers many cases. Are there exceptions to this rule?

Turns out there were tests for this feature, but they were skipped.

[gvanrossum]

That's a bizarre solution. Please roll that back so that it behaves exactly as before and file a separate bug. The issue is way more complicated than that -- every extension module may define "builtin" types (according to this definition).

[elazarg]

I will roll back, but I don't understand the problem. Can you elaborate - what's the ultimate intended behavior?

1. Before this PR, any module could declare @builtinclass type.
2. In the previous version of this PR, only object is considered a builltin, so we don't warn about conflicting hierarchy.
3. In the current solution, extension modules can "declare" builtin types, but they have to add them to the builtin module explicitly, and give them a lowercase name.

What's wrong with option 3 ?

[gvanrossum]

Option 3 is not at all how Python actually works. Adding stuff to the builtins module is not what extension modules should do, and the all-lowercase naming convention only works reliably for a few well-known types like int, str, list.

When there is a layout conflict due to attempting to M.I. from two different extension classes, Python will reject the class definition at runtime, so mypy won't be finding new problems even if it did faithfully emulate the runtime behavior. But the runtime behavior is also impossible to emulate (it requires more information that can be expressed using PEP 484 and the @builtinclass decorator), and I propose not to bother, since it never worked anyway.

We should get rid of the builtinclass decorator from typeshed (there are no uses currently) and of the code in mypy related to is_builtinclass (what the previous version of this PR attempted). We can also get rid of nearest_builtin_ancestor() since the check that uses it does not add extra safety -- M.I. from two builtin classes typcally fails because of conflicting methods anyways. The two tests that expect that error message have code that is rejected for other reasons with the real typeshed -- try my -c 'class A(int, str): pass and see the errors fly by.

So mypy should just not bother trying to detect layout conflicts. The code block in check_multiple_inheritance() labeled # Verify that base class layouts are compatible. can just be deleted, as can the INSTANCE_LAYOUT_CONFLICT message.

Hope this helps!

[elazarg]

It sure helps. Thank you!

[elazarg]

Should I remove the tests altogether then?

[codecov-io]

Current coverage is 83.09% (diff: 100%)

No coverage report found for master at 48fa2ef.

Powered by Codecov. Last update 48fa2ef...b2eaef5

[gvanrossum]

Which tests?

[elazarg]

The skipped tests. Which I removed in the PR.