Extract SassScript functions outside the Compiler class

[stof]

Motivation

Currently, the Compiler class is huge (approx 9kLOC) which makes it harder to work on them:

• identifying the public API is hard
• such big file slows down IDE (my PHPStorm regularly gets stuck at analyzing the code while I type when working in this file, which then prevents using autocompletion)

The current architecture also suffers from some weaknesses:

• the discovery of available functions and their prototype is magic
• all these lib* methods are considered unused by static analysis, because it cannot recognize the way they are used
• overloaded functions (functions with multiple prototypes) have to guess the prototype being used based on the arguments, while the calling code already knows exactly which overload was used
• this architecture would not play well with implementing modules

Proposal

SassScript functions are moved into a bunch of separate (internal) classes in the ScssPhp\ScssPhp\Functions namespace. These functions are grouped by Sass module to which they related (so this namespace would contain Color, Math, List, Map, String, Selector and Meta classes, with a suffix to decide upon to avoid issues as we have 2 reserved keywords in that list of names). Functions will be implemented as public static methods in these classes. Overloads will be able to use separate callables to simplify the implementation. Helper functions will become private static methods in the classes using them, or be moved to a separate Util or to the Value API if they need to be reused across modules.

Some meta functions will still be implemented in the compiler layer because they require access to internals of the evaluation environment (that's what they are meant to expose). For reference, dart-sass also treat them in a special way for that reason. In practice, I think they will use an infrastructure similar to the one of custom functions. Some meta functions can be implemented standalone and will follow the normal pattern though.

Then, for the registration of functions with their signatures, I suggest to have a FunctionRegistry class holding them all. Note that this proposal differs from the dart-sass architecture on purpose to be more friendly to the way PHP works. This class is designed to rely on a big static array that can stay in OPCache shared memory rather than relying on lots of objects that need to be instantiated each time we create a compiler.

namespace ScssPhp\ScssPhp\Functions;

class FunctionRegistry
{
    private static $functions = [
        'str-index' => [ 'arguments' => [ 'string', 'subString' ], 'callback' => [ StringFunctions::class, 'index' ] ],
        'rgb' => [ 'overloads' => [
            [ 'arguments' => [ 'channels' ], 'callback' => [ ColorFunctions::class, 'rgbChannels' ] ],
            [ 'arguments' => [ 'color' , 'alpha'], 'callback' => [ ColorFunctions::class, 'rgbTwoArgs' ] ],
            [ 'arguments' => [ 'red', 'green', 'blue'], 'callback' => [ ColorFunctions::class, 'rgb' ] ],
            [ 'arguments' => [ 'red', 'green', 'blue', 'alpha'], 'callback' => [ ColorFunctions::class, 'rgb' ] ],
        ] ],
        'scale-color' => [ 'arguments' => [ 'color' ], 'restArgument' => 'kwargs', 'callback' => [ ColorFunctions::class, 'scale' ] ],
        // Lots of other functions
    ];

    /**
     * @param string $name The normalized function name (i.e. using dashes, not underscores)
     *
     * @return array|null To be confirmed
     */
    public static getFunction($name)
    {
        // ...
    }
}

The exact type of the return from getFunction remains to be defined. It could either be array|null with the array being what we have in the registry, or it could be some object wrapping it. I might keep the array for performance reasons as that's purely internal. The exact structure of the array for arguments remains to be defined, in order to deal with default values in the most efficient way. restArgument is separate from other arguments as its processing is different. The data structure is meant to represent all the necessary info without needing extra parsing at runtime.

Custom functions registered in the compiler will use a similar storage but in a private property of the Compiler, as they must not be shared between compiler instances. The API for Composer::registerFunction still need to be discussed, to see whether we expose the internal format or whether we expect an argument declaration string that gets parsed (or something else)

Requirements

This proposal depends on #265 so that the implementation of functions would not need to depend on Compiler APIs like $this->reduce() and $this->compileValue anymore (usages of $this->error() will be replaced with throwing SassScriptException, which is meant for that).

This proposal also requires that extending the compiler does not allow overriding or calling lib* methods in a way where removing them triggers a BC break in a minor version. This implies one of these 3 requirements:

• the refactoring is done in the 2.0 release (where BC breaks are OK) => this forces to delay 2.0 until this refactoring is done, which does not help an incremental rewrite
• 2.0 forbids extending the Compiler entirely (making it final)
• 2.0 forbids overriding and calling lib* functions when extending the compiler (could be done by changing all these methods to private in 2.0)

Implementation plan

Once the requirements are met, we can introduce the new registry in parallel of the magic discovery (preferring the new registry by checking it first), migrate functions module by module and then remove the magic discovery (if we don't make the compiler itself final in 2.0, we can actually only deprecate the magic discovery but not remove it, due to the child classes potentially using it).

TODOs

• [ ] Define the representation of arguments in the registry to support default values (which must be represented using scalars or arrays of scalars, not Value, to keep the big registry array a static array)
• [ ] Decide on the return value of FunctionRegistry::getFunction
• [ ] Decide on the API for Compiler::registerFunction

[Cerdic]

I can agree with the final target but I think we can do that in a more incremental way than you’re exposing here.

We could start by moving everything in a separate class but keeping the same format, except for the part that needs to access the compiler - but trying to cut the crap as much as possible.

Then use a discovery function in the compiler that will support both the existing lib* convention and the new one and that can evolves in the same time we are rewriting the container for functions.

In that way there is no BC break, we can have an incremental rewriting allowing to keep everything working and at the end when comes the 2.0 version we are removing the compatibility layer for old lib*

If it seems ok for you I can try to take this refactoring in charge in that way Le 10 nov. 2020 à 01:12 +0100, Christophe Coevoet notifications@github.com, a écrit :

Motivation Currently, the Compiler class is huge (approx 9kLOC) which makes it harder to work on them:

• identifying the public API is hard • such big file slows down IDE (my PHPStorm regularly gets stuck at analyzing the code while I type when working in this file, which then prevents using autocompletion)

The current architecture also suffers from some weaknesses:

• the discovery of available functions and their prototype is magic • all these lib* methods are considered unused by static analysis, because it cannot recognize the way they are used • overloaded functions (functions with multiple prototypes) have to guess the prototype being used based on the arguments, while the calling code already knows exactly which overload was used • this architecture would not play well with implementing modules

Proposal SassScript functions are moved into a bunch of separate (internal) classes in the ScssPhp\ScssPhp\Functions namespace. These functions are grouped by Sass module to which they related (so this namespace would contain Color, Math, List, Map, String, Selector and Meta classes, which a suffix to decide upon to avoid issues as we have 2 reserved keywords in that list of names). Functions will be implemented as public static methods in these classes. Overloads will be able to use separate callables to simplify the implementation. Helper functions will become private static methods in the classes using them, or be moved to a separate Util or to the Value API if they need to be reused across modules. Some meta functions will still be implemented in the compiler layer because they require access to internals of the evaluation environment (that's what they are meant to expose). For reference, dart-sass also treat them in a special way for that reason. In practice, I think they will use an infrastructure similar to the one of custom functions. Some meta functions can be implemented standalone and will follow the normal pattern though. Then, for the registration of functions with their signatures, I suggest to have a FunctionRegistry class holding them all. Note that this proposal differs from the dart-sass architecture on purpose to be more friendly to the way PHP works. This class is designed to rely on a big static array that can stay in OPCache shared memory rather than relying on lots of objects that need to be instantiated each time we create a compiler. namespace ScssPhp\ScssPhp\Functions;

class FunctionRegistry { private static $functions = [ 'str-index' => [ 'arguments' => [ 'string', 'subString' ], 'callback' => [ StringFunctions::class, 'index' ] ], 'rgb' => [ 'overloads' => [ [ 'arguments' => [ 'channels' ], 'callback' => [ ColorFunctions::class, 'rgbChannels' ] ], [ 'arguments' => [ 'color' , 'alpha'], 'callback' => [ ColorFunctions::class, 'rgbTwoArgs' ] ], [ 'arguments' => [ 'red', 'green', 'blue'], 'callback' => [ ColorFunctions::class, 'rgb' ] ], [ 'arguments' => [ 'red', 'green', 'blue', 'alpha'], 'callback' => [ ColorFunctions::class, 'rgb' ] ], ] ], 'scale-color' => [ 'arguments' => [ 'color' ], 'restArgument' => 'kwargs', 'callback' => [ ColorFunctions::class, 'scale' ] ], // Lots of other functions ];

public static getFunction($name) { // ... } } The exact type of the return from getFunction remains to be defined. It could either be array|null with the array being what we have in the registry, or it could be some object wrapping it. I might keep the array for performance reasons as that's purely internal. The exact structure of the array for arguments remains to be defined, in order to deal with default values in the most efficient way. restArgument is separate from other arguments as its processing is different. The data structure is meant to represent all the necessary info without needing extra parsing at runtime. Custom functions registered in the compiler will use a similar storage but in a private property of the Compiler, as they must not be shared between compiler instances). The API for Composer::registerFunction still need to be discussed, to see whether we expose the internal format or whether we expect an argument declaration string that gets parsed (or something else) Requirements This proposal depends on #265 so that the implementation of functions would not need to depend on Compiler APIs like $this->reduce() and $this->compileValue anymore (usages of $this->error() will be replaced with throwing SassScriptException, which is meant for that). This proposal also requires that extending the compiler does not allow overriding or calling lib* methods in a way where removing them triggers a BC break in a minor version. This implies one of these 3 requirements:

• the refactoring is done in the 2.0 release (where BC breaks are OK) => this forces to delay 2.0 until this refactoring is done, which does not help an incremental rewrite • 2.0 forbids extending the Compiler entirely (making it final) • 2.0 forbids overriding and calling lib* functions when extending the compiler (could be done by changing all these methods to private in 2.0)

Implementation plan Once the requirements are met, we can introduce the new registry in parallel of the magic discovery (preferring the new registry by checking it first), migrate functions module by module and then remove the magic discovery (if we don't make the compiler itself final in 2.0, we can actually only deprecate the magic discovery but not remove it, due to the child classes potentially using it). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[mahagr]

I agree with @Cerdic, it is usually better to do refactoring in smaller pieces and to test the changes with releases in between. Moving code out of the Compiler class is easy enough -- I've already done it in my end. It is also a good place to start and even if it breaks code that relies on extending the Compiler class, it is super easy to fix that in the code that uses this library (I'm not considering those methods being as a part of public API).

[stof]

Well, as all these methods are protected, they can be overridden and called by child classes. That's why I suggest that this should be done after 2.0, assuming 2.0 prevent such extension. If we decide that keeping BC on this is not necessary, this could indeed be done earlier.

Then use a discovery function in the compiler that will support both the existing lib* convention and the new one and that can evolves in the same time we are rewriting the container for functions.

See my implementation plan. I'm already suggesting that both systems exist in parallel for some time (potentially over multiple 2.x releases). If the compiler does not allow extension anymore, the lib* discovery becomes purely internal and can be removed even in a minor version once it is not used by our code anymore.

We could start by moving everything in a separate class but keeping the same format, except for the part that needs to access the compiler - but trying to cut the crap as much as possible.

As long as we don't have the value object refactoring, I think most functions require some access to $compiler->reduce(), $compiler->compileValue or other methods dealing with the crappy array-based API. That's why I suggested doing that extraction after the value refactoring.

[mahagr]

I don't think that many users override the SCSS classes. And I can say that with Gantry I've had to refactor overrides anyway couple times already because of the old code didn't quite work anymore. So what comes to the protected methods, they have never been considered part of API and have had minor (breaking changes, but easy to fix) incompatibilities multiple times already.

So I would just make those changes and IF someone comes here to ask about the changes like I did, just point them to use the officially supported approach.

This leaves how to deal with the current way to import functions as well as the array-based api. And hopefully a way to hook into url() content.

[stof]

@mahagr the official way to extend the compiler with more functions is to use registerFunction. The new FunctionRegistry won't be extendable. It is only about core functions. Supporting both the new way based on FunctionRegistry and the lib* discovery will be easy, as we have a single place centralizing that (and the prototype format for lib* can easily be converted into the new format)