Support basic usage of class_alias #586

ebernhardson · 2017-03-09T03:31:25Z

This supports the most basic form of class_alias, which probably
covers most use cases. It adds aliases for class_alias used in the
global scope with constant string arguments.

Fixes #521

TysonAndre

I'm not really sure how complete the other maintainers would want support for class_alias to be before adding it to a release.

Things that could be improved were mentioned in my review comments

TysonAndre · 2017-03-09T03:40:44Z

src/Phan/CodeBase.php

+        FullyQualifiedClassName $alias
+    ) {
+        $class = $this->getClassByFQSEN( $original );
+        $this->fqsen_class_map[$alias] = $class;


Seems ok overall as a first start, might want to have this behind a config (false by default and in .phan/config.php) since it might not be finished for a while.

There can be duplicates of classes.

Also, there might also be a class by the same name as an alias, and

something like PhanDuplicateClass/PhanDuplicateClassAlias should be emit (may or may not check if the strings are equal)

See the other uses of withAlternateId() in Phan when adding class definitions

Also, people can call class_alias above (or in a different file parsed before) a class declaration.

nit: Also, add doc comment with @return void, like the others in the file
nit: also, remove space between ( and first param, remove space between last param and ) in function invocations

Long term, there should be a step after the parseFiles phase but before the analyzeClasses/analyzeMethods phase, when all of the classes have been parsed

And there can technically be a class_alias of class_alias of a class_alias ...., which also makes a complete implementation harder

If people start to alias their aliases like that they are on their own. Having support for a single level of aliasing should be sufficient for most cases.

A single level of aliasing seems fine.

Also, need to check that the original class exists in Phan\CodeBase when adding an alias, emit an PhanUndeclaredClass if it doesn't

I've got most of this worked out, the one problem i'm having is code such as:

<?php class A {} class_alias('A', 'B'); class B {}

The issue generated ends up looking like

%s:4 PhanRedefineClass Class \B defined at %s:4 was previously defined as Class \A at %s:2

Basically, the aliased class reports itself as the original class (like it does in php), but then when it comes time to report the error the original definition is reported as the original class definition, rather than the class_alias() call. It does work appropriately the other way around, when the issue is emitted from the class_alias call rather than from the class definition.

Will update the patch with my latest work, but it won't be ready to merge yet while i ponder this issue.

TysonAndre · 2017-03-09T03:42:23Z

src/Phan/Parse/ParseVisitor.php

+                && isset($args->children[1])
+                && is_string($args->children[1])
+            ) {
+                $originalFqsen = FullyQualifiedClassName::fromFullyQualifiedString( $args->children[0] );


May want to check if $originalFqsen is valid, emit a new issue type here? https://secure.php.net/manual/en/language.variables.basics.php

As a regular expression, it would be expressed thus: '[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*'

I double checked and php actually doesn't care, you can class_alias("Foo", "\0"); and then class_alias("\0", "Bar"); and Bar will be a valid alias (as is $nb = "\0"; $x = new $nb();). While dirty, it's allowed so not sure we need an issue?

TysonAndre · 2017-03-09T03:43:54Z

tests/files/src/0258_class_alias.php

+namespace {
+    class A {}
+    class_alias('A', 'B');
+    $x = new B();


Add a test of an undeclared class in anonymous namespace as well?

Optionally, add a test of this working for traits
Optionally, add a test of something extending an alias class

Add a test of using something from the original (e.g. calling a method of) a class alias (Defined in class A)

morria · 2017-03-09T14:20:34Z

I'm not really sure how complete the other maintainers would want support for class_alias to be before adding it to a release.

As class_alias is a thing that we've heard a few reports of over time as being an annoying edge case, my personal preference is to have something, even if its not perfect.

I like the idea of emitting a PhanDuplicateClass issue if it overwrites a known class. Since we're doing this during the parsing phase, it'll be "random" (or rather, a function of file ordering) if the issue is emitted during the aliasing or at the declaration of the class, but I'm fine with that. Adding a new issue PhanDuplicateClassAlias might be confusing since it'll random if the issue is on the declaration or alias.

Long term, there should be a step after the parseFiles phase but before the analyzeClasses/analyzeMethods phase, when all of the classes have been parsed

Phan does a funny thing now which is to defer "hydration" of classes until the class is requested. We used to scan all classes before the analysis phase, but we got a nice speedup by deferring it until after Phan has forked off the sub-processes for analysis.

If we choose to do that hydration serially so that we have full access to the classes before analysis, we should just be aware of the hit on speed we'll be taking.

And there can technically be a class_alias of class_alias of a class_alias ...., which also makes a complete implementation harder

If we get bug reports from folks saying that something broke when they aliases class_alias, I'll be fine with saying that we can't help them in this situation.

ebernhardson · 2017-03-31T23:02:24Z

Took me awhile to have the time to get back to this, but I've updated the patch for various reviews:

Now uses an analysis pass after parsing. This was needed to emit duplicate class warnings on the class_alias call instead of confusing warnings from the definition to the aliased definition. I just couldn't make alternate id's work for this case.
Handles the ::class constant as well as string arguments
Passing the original object into something typed with the alias now works

Problems:

Still trying to figure out why, but running the tests with --filter '0271' runs the test on its own and works. But running the full test suite without filter fails on test 0271 (the new class_alias test). Advice appreciated.

TysonAndre · 2017-03-31T23:15:13Z

Still trying to figure out why, but running the tests with --filter '0271' runs the test on its own and works. But running the full test suite without filter fails on test 0271 (the new class_alias test). Advice appreciated.

You need to isolate the names used in a unit test. Phan runs unit tests on a group of files, not on files individually (The state isn't cleared after each test file)

This is partly done for performance, re-parsing information for all of the internal classes and functions for each file, and invalidating cached state, would be slow.

Instead of A, B, C, I'd suggest using the names A271, B271, C271 for the new test classes (Because 271 is the number for this unit test) (and global functions). (Or something specific to what's being tested, AAliasSource, BAliased, etc.)

TysonAndre · 2017-03-31T23:18:38Z

tests/files/src/0271_class_alias.php

+    testB(new Z());
+}
+
+namespace Foo {


prefer Foo271 here, A271 above, this may conflict with other tests for namespace foo

TysonAndre · 2017-03-31T23:18:52Z

tests/files/src/0271_class_alias.php

+    class_alias('A', 'B');
+    class Z extends B {}
+
+    function testA(A $x) {}


That was exactly it, thanks! Tests passing locally now.

TysonAndre · 2017-03-31T23:21:21Z

src/Phan/Language/Type.php

@@ -992,6 +992,16 @@ public function asExpandedTypes(
                }
            }

+            // Add in aliases
+            $fqsenAliases = $code_base->getClassAliasesByFQSEN($class_fqsen);


asExpandedType seems like the wrong place, it converts Subclass to Subclass|BaseClass.

It'd make more sense to me if aliases were replaced with the original type(s), instead of adding aliases

Type::make() might work (But that returns only one type)

EDIT: asExpandedType might end up being the best way to handle multiple possible original classes for a given class, not completely sure

Unfortunately make doesn't have CodeBase, and is called from plenty of places that don't pass around CodeBase. It would be pretty invasive to get the right information into make for it to resolve the alias back into the original class name.

Okay, Type::make doesn't work. It needs to be at a higher level. This is out of scope of this PR.

It should be possible to do this in a way similar to the way phan handles imports use SomeNamespace\Foo as Bar (Phan sees Bar, but expands it to SomeNamespace\Foo everywhere - instanceof checks, parameters, phpdoc parameters, etc.)

All of the methods such as UnionType::fromNode(context, code_base, children) (Called in AnalysisVisitor), UnionType::fromStringInContext(type, context, true), etc. , all get a Context, which contains a link to the codebase.

See the implementation of Type::fromStringInContext, from the // Check to see if the type name is mapped via a using clause. part of the code.
(This would be checked after use statements are checked, and possibly in combination with a use statement)

TysonAndre · 2017-03-31T23:26:01Z

src/Phan/CodeBase.php

+            $fqsenAliases = [];
+        }
+        $fqsenAliases[] = $alias;
+        $this->fqsen_alias_map[$original] = $fqsenAliases;


This list may end up with duplicates if multiple files define the same alias. A map to a set of fqsens may make more sense

Set makes sense here, I've switched it over.

This supports the most basic form of class_alias, which probably covers most use cases. It adds aliases for class_alias used in the global scope with constant string arguments or the class referenced via the ::class constant. Fixes phan#521

TysonAndre

This seems pretty close to done.

Code style is consistent with what we already have, and the code is well commented.

This is reading files from disk much more often than necessary. See my comment on src/Phan/Analysis.php

The step to extract the occurences of class_alias seems like it should be merged into the Parse phase completely. As this already does, the extracted occurences should be resolved after the Parse phase is completely over and classes are created.

RedefineClass may make more sense if split into RedefineClassAlias/RedefineAliasedClass, so that users are able to better fine tune which issues to suppress in large codebases.

TysonAndre · 2017-04-09T03:02:16Z

src/Phan/Analysis.php

+    ) {
+        foreach ($file_path_list as $i => $file_path) {
+            try {
+                AliasAnalyzer::parseFile($code_base, $file_path);


The way this is written, this will read every single file from disk, and call \ast\parse_code twice.
This is inefficient, and will slow phan down (Especially the parsing from disk part).

most files in a typical process have 0 class_alias statements, but this step would parse them anyway.

a small fraction of lines in a file with class_alias are actually class_alias

On another note, I think that class_alias should take effect for all parsed files, not just the subset which is analyzed. Class declarations, function declarations, etc. are all parsed from parsed files, and creating classes from an alias is somewhat similar.

A preferred solution would be:

During the parse phase, when class_alias is encountered, add information with the context, source class name, and target class name to this.
E.g. create \Phan\Language\Element\ClassAlias with the properties sourceClassName, targetClassName, and maybe Context and maybe \ast\Node $node
Add those to a list
(Not 100% sure if the context is necessary, but MyClass::class would be a different string in namespace Foo. Actually, the file and line might be needed to warn about bad aliases (e.g. missing source class). $node probably isn't needed either.)

During the analyze phase, loop over that list of ClassAlias, and perform the AliasAnalyzer steps
(The step to create aliases seems short enough to fit in one or two methods of the Analysis class.)

Addressed in PR 907

TysonAndre · 2017-04-09T03:08:17Z

src/Phan/Analysis/AliasAnalyzer.php

+
+        // Recurse into each child node
+        $child_context = $context;
+        foreach ($node->children ?? [] as $child_node) {


This should be moved into the parse phase

You'll need to track these somewhere. I suggest $code_base->recordClassAlias(sourceClass, targetClass, etc.)

Addressed in PR 907

TysonAndre · 2017-04-09T03:09:27Z

src/Phan/Analysis/AliasVisitor.php

+        $expression = $node->children['expr'];
+
+        if ($expression->kind !== \ast\AST_NAME
+            || $expression->children['name'] !== 'class_alias'


nit: strcasecmp. The other code is a bad example, but Phan should really be using strcasecmp(name, 'fnname') === 0 in a lot of places.

(If it's invoked as CLASS_ALIAS or some other weird case-insensitive equivalent).

Addressed in PR 907

TysonAndre · 2017-04-09T03:20:23Z

src/Phan/Analysis/AliasVisitor.php

+        } else if ($this->code_base->hasClassWithFQSEN($alias_fqsen)) {
+            $clazz = $this->code_base->getClassByFQSEN($alias_fqsen);
+            $this->emitIssue(
+                Issue::RedefineClass,


Would prefer a new issue type RedefineClassAlias with the same category/severity. I don't really care as much about what happens if class and alias conflict (for this PR), mostly mention this for the case of alias and alias conflict.

These are all done at the level of the global scope, and phan doesn't have a way to suppress an individual statement outside of a function/method.

Users may want a way to check for duplicate classes without checking for duplicate aliases, or vice versa
(E.g. Same alias may exist in different files).

Addressed in PR 907

TysonAndre · 2017-04-09T03:21:45Z

src/Phan/Analysis/AliasVisitor.php

+                Issue::RedefineClass,
+                $node->lineno ?? 0,
+                $args->children[1],
+                $this->context->getFile(),


The suggestion I made would involve taking this context and file/line from the AliasInfo

Addressed in PR 907

TysonAndre · 2017-04-09T03:24:38Z

tests/files/src/0278_class_alias.php

+    class Dupe {}
+    trait OriginalTrait {}
+
+    class_alias('\Foo278\NamespacedOriginal', 'AliasedFromNamespace278');


Add a test with NamespacedOriginal::class passed to the source of class_alias

php > namespace C{echo A::class;} C\A

TysonAndre · 2017-04-14T06:14:26Z

Additionally, after #563 (Phan Daemon Mode) is merged, this PR will have to be modified.

Class aliases would have to be removed if they were removed from a file which was removed (Or if the file was edited and a line was removed)
Class aliases would have to be added if that line was added to a file.

See https://github.com/etsy/phan/pull/563/files#diff-8337a4d0a543840186ee3e227bebbd96 and https://github.com/etsy/phan/pull/563/files#diff-c751b0fec138a443efefd42213266740 , as well as the uses of this.

Also, after rebasing, this PR should mention class_alias support in the NEWS file in the top level directory in the notes for the latest 0.9.x-dev code.

This PR is based off of phan#586 - Account for daemon mode (Not fully tested with the config setting on yet) - Parse class_alias original and alias FQSENs in one phase, and create the entries in another phase (Keeps the number of times each file is parsed the same as before) - Follow variable naming conventions - Tests continue to pass.

TysonAndre · 2017-06-18T19:57:55Z

This PR had merge conflicts and pending review comments for a while.

#907 (which is based on this PR) fixed the merge conflicts, and most of the remaining review comments.

That PR was merged to the master branch. In order to analyze class_alias statements in the global scope, use the master branch (0.9.3-dev) with the following addition to .phan/config.php

    // If true, Phan will read `class_alias` calls in the global scope,
    // then (1) create aliases from the *parsed* files if no class definition was found,
    // and (2) emit issues in the global scope if the source or target class is invalid.
    // (If there are multiple possible valid original classes for an aliased class name,
    //  the one which will be created is unspecified.)
    // NOTE: THIS IS EXPERIMENTAL, and the implementation may change.
    'enable_class_alias_support' => true,

TysonAndre reviewed Mar 9, 2017

View reviewed changes

morria assigned TysonAndre and unassigned TysonAndre Mar 9, 2017

ebernhardson force-pushed the class_alias branch from 23dddc5 to 705416a Compare March 10, 2017 01:33

ebernhardson force-pushed the class_alias branch from 705416a to ef0a9c1 Compare March 31, 2017 22:50

TysonAndre reviewed Mar 31, 2017

View reviewed changes

ebernhardson force-pushed the class_alias branch 2 times, most recently from ab7ec83 to 6cd61f4 Compare April 1, 2017 00:28

Support basic usage of class_alias

a035257

This supports the most basic form of class_alias, which probably covers most use cases. It adds aliases for class_alias used in the global scope with constant string arguments or the class referenced via the ::class constant. Fixes phan#521

ebernhardson force-pushed the class_alias branch from 6cd61f4 to a035257 Compare April 1, 2017 00:39

TysonAndre requested changes Apr 9, 2017

View reviewed changes

TysonAndre closed this Jun 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support basic usage of class_alias #586

Support basic usage of class_alias #586

ebernhardson commented Mar 9, 2017

TysonAndre left a comment •

edited

Loading

TysonAndre Mar 9, 2017

TysonAndre Mar 9, 2017

rlerdorf Mar 9, 2017

TysonAndre Mar 9, 2017

ebernhardson Mar 10, 2017

TysonAndre Mar 9, 2017

ebernhardson Mar 10, 2017

TysonAndre Mar 9, 2017

morria commented Mar 9, 2017

ebernhardson commented Mar 31, 2017

TysonAndre commented Mar 31, 2017 •

edited

Loading

TysonAndre Mar 31, 2017

TysonAndre Mar 31, 2017

ebernhardson Apr 1, 2017

TysonAndre Mar 31, 2017 •

edited

Loading

ebernhardson Apr 1, 2017

TysonAndre Apr 9, 2017

TysonAndre Mar 31, 2017

ebernhardson Apr 1, 2017

TysonAndre left a comment

TysonAndre Apr 9, 2017

TysonAndre Jun 18, 2017

TysonAndre Apr 9, 2017

TysonAndre Jun 18, 2017

TysonAndre Apr 9, 2017

TysonAndre Jun 18, 2017

TysonAndre Apr 9, 2017

TysonAndre Jun 18, 2017

TysonAndre Apr 9, 2017

TysonAndre Jun 18, 2017

TysonAndre Apr 9, 2017

TysonAndre commented Apr 14, 2017 •

edited

Loading

TysonAndre commented Jun 18, 2017

Support basic usage of class_alias #586

Support basic usage of class_alias #586

Conversation

ebernhardson commented Mar 9, 2017

TysonAndre left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morria commented Mar 9, 2017

ebernhardson commented Mar 31, 2017

TysonAndre commented Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TysonAndre Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TysonAndre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TysonAndre commented Apr 14, 2017 • edited Loading

TysonAndre commented Jun 18, 2017

TysonAndre left a comment •

edited

Loading

TysonAndre commented Mar 31, 2017 •

edited

Loading

TysonAndre Mar 31, 2017 •

edited

Loading

TysonAndre commented Apr 14, 2017 •

edited

Loading