[RFC][Ast][Routing] Use Ast to generate the UrlGenerator #19555

GuilhemN · 2016-08-06T15:23:46Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets
License	MIT
Doc PR

This PR applies the bases proposed in #17516 to the Routing component.
Generating a Normalizer is complicated (i think that's worth it but that's not ideal to introduce a new component) so I decided to try to generate the cached matcher/generator of the routing component with AST.
I created an util class to easily convert simple php code to an ast node (convenient when you have a part of your code which never changes dynamically).
I didn't finish this PR because I'm waiting for the core team approval.

The main advantages of using ast I see:

Coding standards are managed by the code generator
It's easier to generate complex code
The user can optimize/update and do whatever he wants on the ast without dirty hacks

ping @joelwurtz

joelwurtz · 2016-08-06T15:29:28Z

src/Symfony/Component/AstGenerator/Util/AstHelper.php

+     * @return Expr
+     */
+    public static function value($value)
+    {


Why not directly convert the value to ast with the parser, by using var_export (as all the logic is already handled) ?

You mean passing the raw value to the Nodes directly ? This is managed by the Builders but not by the Exprs.

I forget to mention to use var_export, that's what i'm using here : https://github.com/jolicode/jane-openapi/blob/65fe879139b8eaad18d132e850cebadd85f88dca/src/Generator/Parameter/NonBodyParameterGenerator.php#L67

I'm not sure it would support well every edge cases such as non-associative array (var_export always prints the keys if i'm not wrong).
BTW this part of the code is extracted from nikic's library.

unkind · 2016-08-06T17:33:58Z

src/Symfony/Component/AstGenerator/AstGeneratorChain.php

+ *
+ * @author Joel Wurtz <joel.wurtz@gmail.com>
+ */
+class AstGeneratorChain implements AstGeneratorInterface


Why this class is open for inheritance (not marked as final)?
Same question about UrlGeneratorGenerator, PhpMatcherGenerator, UniqueVariableScope.

For this class and UniqueVariableScope i agree it would make sense even if there aren't that much final classes in symfony.
But for the routing generators i would like to allow the user to easily hook into them and it could pass by inheritance.

But for the routing generators i would like to allow the user to easily hook into them and it could pass by inheritance.

How? Those classes have only private members and 2 public methods. And those 2 public methods can be easily changed with composition if developer really needs it.
This is weird Symfony internal rule: to keep members private by default, but in the same time do not require final: there is no point to open them for inheritance then.

If you find valid reason to extend the classes, you can provide sane extension point(s) instead of inheritance: strategies, events, hooks, etc.

For now they are private but it may change. But you're right the class should be final as long as there are no extensions possible, i'll change ASAP.

For now they are private but it may change.

Sure, but instead of changing private properties to protected, you probably can introduce better extension point.

Thx. I wasnt really opinionated on it, yet. https://ocramius.github.io/blog/when-to-declare-classes-final/ is also a nice read. Anyway, it looks good practice to me.. not sure if it's common enough (yet) for symfony though.

@Ener-Getick then maybe it would be better to lock them (final and @internal) first no?

Once you fix it, our comments will disappear :)

@theofidry i'm using my phone tonight :P

Good thing, you shouldn't work a saturday evening anyway ;)

theofidry · 2016-08-06T20:50:57Z

@Ener-Getick looking at it most of it comes from #17516 and then you apply it to the routing component. To keep things smaller, wouldn't it be better to finish #17516 rather?

GuilhemN · 2016-08-06T20:59:30Z

@theofidry the component itself is more or less ready but it isn't especially useful if we don't use it in symfony.
And generating normalizers is complicated as the serializer provides many features so i thought it would be better to do something smaller as a first step.

theofidry · 2016-08-06T21:08:31Z

the component itself is more or less ready but it isn't especially useful if we don't use it in symfony

Then #17516 could be merged somehow no? It would need to be locked first (finals and @internal), but polishing the component will come with more usage (like this PR).

But I'm saying that to simplify this PR, if you feel it's not needed it's alright.

GuilhemN · 2016-08-06T21:13:36Z

Some parts of #17516 aren't ready to be merged and would make sf tests fail so i don't think it would be accepted, even tagged as @internal

unkind · 2016-08-06T22:57:35Z

src/Symfony/Component/Routing/Matcher/Generator/PhpMatcherGenerator.php

+            ->addStmt($this->generateMatchMethod($object, $context));
+    }
+
+    private function generateMatchMethod($object, array $context)


Both arguments are unused.

joelwurtz · 2016-08-07T00:40:59Z

While the AstGeneratorInterface make sense for generating normalizer and types (as we have different kinds of objects and metadata), it does not make sense here like @unkind said, as we always have the same input.

Given this, the ASTGenerator Component does not add a value in this context (just use php-parser).

I have begin the work on the AstGeneratorComponent for the purpose of generating better and faster normalizer like in #17516

Then the idea comes to also apply AST generation to components already generating code (like the router or the DIC).

Seeing this pr from @Ener-Getick and comments made me realise that we don't need to do an ASTGenerator Component, but instead separate the generation of each component into itself.
(So the AstGeneratorInterface will be renamed and include in the Serializer Component).

However this does not mean we should stop working on ASTGenerator Component, instead we must renaming it to ASTComponent which will provides utility class that works on a AST (a.k.a. NodeList):

Debug AST Generation
AST Diff (useful for debug)
AST Optimization
Helper class ?

Also we should provide a unified way of writing AST into the differents components (not speaking about interface, but more about the choices and being consistent of the way of doing across components)

GuilhemN · 2016-08-07T09:20:38Z

@joelwurtz I agree the ast component would indeed be more valuable like that.
It could provide a printer compliant to symfony practices and util classes allowing to easily generate code.
I will do that ASAP :)

joelwurtz · 2016-08-07T09:26:01Z

No need for a printer there is already php cs fixer :)

GuilhemN · 2016-08-07T09:31:01Z

@joelwurtz it's not part of symfony and won't be applied to the code cached anyway so i think it makes sense to have our own printer (based on the one included in PHP-Parser).
At least, we should have one wrapping the default printer to support NodeList imo:

class AstDumper
{
    public function __construct(PrettyPrinterAbstract $basePrinter = null);
    public function dump(NodeList $nodes);
}

unkind · 2016-08-07T12:42:15Z

src/Symfony/Component/Routing/Generator/AstGenerator/GeneratorAstGenerator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function generate(array $options = array())


Ironically, I'd swap options and RouteCollection.

We have RouteCollection and we want to generate AST for this route collection. It looks natural to pass it as an argument of the generate($routeCollection) method, doesn't it?

I reused the current signature. I have no strong opinion about passing the route collection but it looks weird to me to not know the name of the class that will be generated (most of the time the caller doesn't instantiate the class it calls).

By analogy, why do you pass NodeList in the dump method instead of constructor in example above (AstDumper)? Does it look weird that you don't know printer parameters that will be used?

You can pass options as 2nd and 3rd arguments if you think you need to pass them explicitly:

public function generate( RouteCollection $routeCollection, string $class = 'ProjectUrlGenerator', string $baseClass = UrlGenerator::class )

If you take a look on your interface below, it just says that it generates AST for routes, but it wonders me which routes description is talking about?

For me this dumper can be used to print different files (you can use the same config to print them without combining them) this is why i think the ast should be passed in the dump method.

About the generate method that's true, i only have to find how it is used exactly in the framework to ensure we can prefer explicitness without creating issues.

nicolas-grekas · 2016-08-09T09:47:16Z

I don't see much value in using an AST-based code generator here.
The resulting code will be the same and it adds complexity (thus harder to contribute/fix) for an already solved problem.
I tend to agree with @joelwurtz in that if the envisioned ast generator is targeted at generating optimized normalizers, it should be in the Serializer component directly.
We don't need a generic code generator component IMHO.
Thus 👎

GuilhemN · 2016-08-09T15:19:06Z

@nicolas-grekas i was thinking allowing the user to hook into the process and eventually change the generate method behavior but maybe that shouldn't be allowed (the user should create his own generator).
BTW we can decrease this code complexity by parsing a template with PHP-parser. We would still be able to benefit of a code optimization (or rewriting for supporting features of newer php versions) without the complexity but in that case we will have to create a component managing this optimization part.

So maybe after all that's better to begin with something where ast brings a clear improvement and then we will see if it should be extanded to other components or not.
I'm closing this one for now and will maybe reopen it if #17516 is well accepted.

carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) Routing Feature labels Aug 6, 2016

GuilhemN force-pushed the DUMPERGENERATOR branch from 1abbd58 to b039808 Compare August 6, 2016 15:25

joelwurtz and others added 2 commits August 6, 2016 17:26

[AstGenerator] Init the Component

c2d733a

[AstGenerator] Create an util class to instantiate common nodes

8e89d34

joelwurtz reviewed Aug 6, 2016
View reviewed changes

GuilhemN force-pushed the DUMPERGENERATOR branch from b039808 to d1f1588 Compare August 6, 2016 15:29

GuilhemN added 2 commits August 6, 2016 17:39

[Routing] Convert the PhpMatcherDumper to an AstGeneratorInterface

0b61e4c

[Routing] Convert the PhpGeneratorDumper to an AstGeneratorInterface

d258187

unkind reviewed Aug 6, 2016
View reviewed changes

GuilhemN force-pushed the DUMPERGENERATOR branch from d1f1588 to 90705e8 Compare August 7, 2016 09:55

Focus on GeneratorAstGenerator

791a955

GuilhemN force-pushed the DUMPERGENERATOR branch from 90705e8 to 791a955 Compare August 7, 2016 10:03

GuilhemN changed the title ~~[RFC][AstGenerator][Routing] Convert the PhpMatcherDumper to an AstGeneratorInterface~~ [RFC][Ast][Routing] Use Ast to generate the UrlGenerator Aug 7, 2016

unkind reviewed Aug 7, 2016
View reviewed changes

nicolas-grekas mentioned this pull request Aug 9, 2016

[AstGenerator] [WIP] New Component, add normalizer generator #17516

Closed

7 tasks

GuilhemN closed this Aug 9, 2016

GuilhemN deleted the DUMPERGENERATOR branch August 9, 2016 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][Ast][Routing] Use Ast to generate the UrlGenerator #19555

[RFC][Ast][Routing] Use Ast to generate the UrlGenerator #19555

GuilhemN commented Aug 6, 2016

joelwurtz Aug 6, 2016 •

edited

Loading

GuilhemN Aug 6, 2016

joelwurtz Aug 6, 2016

GuilhemN Aug 6, 2016 •

edited

Loading

unkind Aug 6, 2016 •

edited

Loading

GuilhemN Aug 6, 2016

unkind Aug 6, 2016 •

edited

Loading

GuilhemN Aug 6, 2016

unkind Aug 6, 2016

ro0NL Aug 6, 2016 •

edited

Loading

theofidry Aug 6, 2016

unkind Aug 6, 2016

GuilhemN Aug 6, 2016

theofidry Aug 6, 2016

theofidry commented Aug 6, 2016

GuilhemN commented Aug 6, 2016 •

edited

Loading

theofidry commented Aug 6, 2016

GuilhemN commented Aug 6, 2016

unkind Aug 6, 2016

joelwurtz commented Aug 7, 2016 •

edited

Loading

GuilhemN commented Aug 7, 2016

joelwurtz commented Aug 7, 2016

GuilhemN commented Aug 7, 2016 •

edited

Loading

unkind Aug 7, 2016

GuilhemN Aug 7, 2016 •

edited

Loading

unkind Aug 7, 2016 •

edited

Loading

GuilhemN Aug 7, 2016

nicolas-grekas commented Aug 9, 2016 •

edited

Loading

GuilhemN commented Aug 9, 2016

[RFC][Ast][Routing] Use Ast to generate the UrlGenerator #19555

[RFC][Ast][Routing] Use Ast to generate the UrlGenerator #19555

Conversation

GuilhemN commented Aug 6, 2016

joelwurtz Aug 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN Aug 6, 2016 • edited Loading

Choose a reason for hiding this comment

unkind Aug 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkind Aug 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Aug 6, 2016 • 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

theofidry commented Aug 6, 2016

GuilhemN commented Aug 6, 2016 • edited Loading

theofidry commented Aug 6, 2016

GuilhemN commented Aug 6, 2016

Choose a reason for hiding this comment

joelwurtz commented Aug 7, 2016 • edited Loading

GuilhemN commented Aug 7, 2016

joelwurtz commented Aug 7, 2016

GuilhemN commented Aug 7, 2016 • edited Loading

Choose a reason for hiding this comment

GuilhemN Aug 7, 2016 • edited Loading

Choose a reason for hiding this comment

unkind Aug 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 9, 2016 • edited Loading

GuilhemN commented Aug 9, 2016

joelwurtz Aug 6, 2016 •

edited

Loading

GuilhemN Aug 6, 2016 •

edited

Loading

unkind Aug 6, 2016 •

edited

Loading

unkind Aug 6, 2016 •

edited

Loading

ro0NL Aug 6, 2016 •

edited

Loading

GuilhemN commented Aug 6, 2016 •

edited

Loading

joelwurtz commented Aug 7, 2016 •

edited

Loading

GuilhemN commented Aug 7, 2016 •

edited

Loading

GuilhemN Aug 7, 2016 •

edited

Loading

unkind Aug 7, 2016 •

edited

Loading

nicolas-grekas commented Aug 9, 2016 •

edited

Loading