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

Improve generated names for anonymous classes #5153

Closed
wants to merge 2 commits into from

Conversation

@nikic
Copy link
Member

nikic commented Feb 6, 2020

In order of preference, the generated name will be:

new class extends ParentClass {};
// -> ParentClass@anonymous
new class implements FirstInterface, SecondInterface {};
// -> FirstInterface@anonymous
new class {};
// -> class@anonymous

This is intended to display a more useful class name in error messages and stack traces, and thus make debugging easier.

There is a minor backwards compatibility break here, in that code currently checking for a class@anonymous prefix should now check for @anonymous instead.

This change is based on the suggestion of @nicolas-grekas in #5143.

@nikic nikic force-pushed the nikic:anon-class-names branch from 305128f to 65a7822 Feb 6, 2020
@PeeHaa

This comment has been minimized.

Copy link
Contributor

PeeHaa commented Feb 6, 2020

Isn't there a sane-ish way to somehow present both interfaces?

It seems pretty arbitrary to pick the first one.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor

nicolas-grekas commented Feb 6, 2020

I couldn't come up with a better proposal. Can you? Listing all interfaces can be pretty long very quickly, especially when considering namespaces. In practice, the first is often the most useful, so it's not that arbitrary to me.

@PeeHaa

This comment has been minimized.

Copy link
Contributor

PeeHaa commented Feb 6, 2020

I do agree it will get long fast. Not sure I have a better suggestion at this point besides somehow mashing the interfaces together with a delimiter.

In practice, the first is often the most useful

I don't understand why this would be the case. Unless I am missing your point the order of interfaces carries no weight one way or another.

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 6, 2020

I don't understand why this would be the case. Unless I am missing your point the order of interfaces carries no weight one way or another.

I think it's not so much that the first is the most useful, but that in the majority of the cases there will be only a single parent or a single interface, so this heuristic works well in practice.

We could of course include both the parent and all interfaces, and for that matter the file and line where it is declared, but ... that would make the name very long and unusable for error messages. We have to strike a reasonable compromise between providing relevant information and being concise here, and I think the approach proposed here is that compromise.

@mfn

This comment has been minimized.

Copy link

mfn commented Feb 6, 2020

Here's an example of a project where I added tests where anonymous classes within tests are used: https://github.com/spatie/data-transfer-object/pull/81/files#diff-1e8fc8e188715be5ac191bc06e73ade9R30

I believe this one will break after this change (the anonymous class "extends" an actual class).

However this specific test already uses expectExceptionMessageRegExp so I guess I just need to adapt it and it's fine 👍

@PeeHaa

This comment has been minimized.

Copy link
Contributor

PeeHaa commented Feb 6, 2020

but that in the majority of the cases there will be only a single parent or a single interface, so this heuristic works well in practice

Which is totally fair as long as it is clear to people the arbitrary choice is made so that it actually is more clear instead of people getting confused (which I suspect would be the case when they first encounter it).

@guilliamxavier

This comment has been minimized.

Copy link
Contributor

guilliamxavier commented Feb 7, 2020

Listing all interfaces can be pretty long very quickly, especially when considering namespaces

About that, could you please add a test with a namespaced base, just to make it clear whether e.g. \get_class(new class() extends \My\Ns\Foo {}) will be 'My\Ns\Foo@anonymous' . "\0" . <unique_stuff> or 'Foo@anonymous' . "\0" . <unique_stuff>?

nikic added 2 commits Feb 6, 2020
In order of preference, the generated name will be:

    new class extends ParentClass {};
    // -> ParentClass@anonymous
    new class implements FirstInterface, SecondInterface {};
    // -> FirstInterface@anonymous
    new class {};
    // -> class@anonymous

This is intended to display a more useful class name in error messages
and stack traces, and thus make debugging easier.
@nikic nikic force-pushed the nikic:anon-class-names branch from 65a7822 to 6cad62b Feb 7, 2020
@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 7, 2020

@guilliamxavier I've added a test for that. The namespace is (of course) included.

@naitsirch

This comment has been minimized.

Copy link

naitsirch commented Feb 7, 2020

We could of course include both the parent and all interfaces, and for that matter the file and line

I would say that file and line would be most helpful for debugging purposes.

What happens if multiple anonymous classes extending the same parent class are declared one after the other?

@php-pulls php-pulls closed this in 72bd559 Feb 17, 2020
@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 17, 2020

@naitsirch FWIW, the file and line are included in the name: The full name (even after this change) is something like ParentClass@anonymous\0/some/long/path/file.php:42$123. Everything after the \0 is cut off in most contexts though, because the resulting name would be too unwieldy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.