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

Reduce the number of exposed symbols #1021

Open
theofidry opened this issue Apr 28, 2024 · 1 comment
Open

Reduce the number of exposed symbols #1021

theofidry opened this issue Apr 28, 2024 · 1 comment

Comments

@theofidry
Copy link
Member

Description

By default, global symbols are marked as exposed. The reason being this allows to easily make any code shipping with polyfills work out of the box. For example:

<?php

if (!function_exists('str_starts_with')) {
  function str_starts_with(...) {...}
}

Will be scoped into:

<?php

namespace RandomPrefix;

if (!function_exists('str_starts_with') && !function_exists('RandomPrefix\str_starts_with')) {
  function str_starts_with(...) {...}
}

// in scoper-autoload.php
if (!function_exists('str_starts_with') {
  function str_starts_with() {
    return \RandomPrefix\str_starts_with(...func_get_args());
  }
}

Notice however, that having str_starts_with exposed, would only be necessary if there is a call that is ambiguous:

<?php

namespace {
  str_starts_with(...);
}

namespace N1 {
  str_starts_with(...);
}

namespace N2 {
  use function str_starts_with;

  str_starts_with(...);
}

Will be scoped into:

<?php

namespace RandomPrefix {
  str_starts_with(...); // correctly resolves to `\RandomPrefix\str_starts_with`
}

namespace RandomPrefix\N1 {
  // Problematic case!
  str_starts_with(...); // resolves to `\str_starts_with` since `\RandomPrefix\N1\str_starts_with` does not exist
}

namespace RandomPrefix\N2 {
  use function RandomPrefix\str_starts_with;

  str_starts_with(...);
}

Currently PHP-Scoper records anytime it comes across a global symbol to expose it. But as demonstrated above, it is only necessary in some scenarios.

Solution

It would be better for PHP-Scoper to track more specific cases that it comes across. For example here, we only need to expose str_starts_with if an ambiguous call is found, it is otherwise unnecessary.

Further considerations:

  • recordedFunctions is currently a bit misleading and with this feature should be renamed. Indeed we want to record the function declarations.
  • for each recorded function declaration, if the function is exposed, an alias should be created (this is what is currently done but requires some changes as the existing recordedFunctions is re-purposed.
  • for each ambiguous function called found:
    • if no global declaration is found, there is nothing to do *1
    • if a global declaration is found:
      • if the symbol is exposed, an alias is created for the global symbol (currently the case) *2 (exposed case)
      • if the symbol is not exposed, only the necessary aliases should be made *2 (not exposed case)
*1 ambiguous calls found but no global declaration
<?php

namespace {
    foo();
}

namespace App {
    foo();
    bar();
}

// scoped version:
namespace Prefix {
    foo();
}

namespace Prefix\App {
    foo();
    bar();
}

// no `\foo()` or `\bar()` function declaration found
// during the scoping, either those are internal symbols
// or there is a namespaced declaration.
// No PHP-Scoper alias needed.

Recorded function declarations: []
Ambiguous function calls:

  • foo: can be either \foo or App\foo
  • bar: can be either \bar or App\bar

Calculating aliases: no function declaration => no alias.

*2 ambiguous calls found with function declarations
<?php

namespace {
    function foo() {};

    foo();
}

namespace App {
    function bar() {};

    foo();
    bar();
}

// scoped version:
namespace Prefix {
    function foo() {};

    foo();
}

namespace Prefix\App {
    function bar() {};

    foo();
    bar();
}

// scoper-autoload.php
// if exposed:
function_alias('foo', 'Prefix\foo');
function_alias('App\bar', 'App\Prefix\foo'); // <- not necessary for it to work, purely because it is exposed

// if not exposed:
function_alias('Prefix\App\foo', 'Prefix\foo');

Recorded function declarations:

  • \foo
  • \App\bar
    Ambiguous function calls:
  • foo: can be either \foo or App\foo
  • bar: can be either \bar or App\bar
  • (another case): bar: can be either \bar or Ppa\bar

Calculating aliases:

  • for foo, \foo was declared, so if foo is exposed, alias [foo, Prefix\foo].
    Otherwise, if needs to be aliases for each case:
    • [Prefix\foo, Prefix\App\foo]
  • for bar, App\bar was declared, so if bar is exposed, alias [App\bar, Prefix\App\bar]
    Otherwise, if needs to be aliases for each case:
    • [Prefix\App\bar, Prefix\Ppa\bar]
*3 reviewing the case of polyfills (no usage found)

In this example str_starts_with is marked as internal.

<?php

namespace {
    if (!function_exists('str_starts_with')) {
        function str_starts_with() {}
    }
}

// scoped version:
namespace Prefix {
    if (!function_exists('str_starts_with') && !function_exists('Prefix\str_starts_with')) {
        function str_starts_with() {}
    }
}

// scoper-autoload.php
// if exposed:
function_alias('str_starts_with', 'Prefix\str_starts_with');

// if not exposed:
// no alias

In this case where no usage is found and the str_starts_with and if not exposed no additional noise is added.

*3 reviewing the case of polyfills (usages found)

In this example str_starts_with is marked as internal.

<?php

namespace {
    if (!function_exists('str_starts_with')) {
        function str_starts_with() {}
    }
}

namespace Amb {
    str_starts_with();
}

namespace FQ {
    \str_starts_with();
}

// scoped version:
namespace Prefix {
    if (!function_exists('str_starts_with') && !function_exists('Prefix\str_starts_with')) {
        function str_starts_with() {}
    }
}

namespace Prefix\Amb {
    str_starts_with();
}

namespace Prefix\FQ {
    \str_starts_with();
}

// scoper-autoload.php
// if exposed:
function_alias('str_starts_with', 'Prefix\str_starts_with');

// if not exposed:
function_alias('Prefix\Amb\str_starts_with', 'Prefix\str_starts_with');
// breaks the case of Prefix\FQ.

To implement the solution

This requires the following information:

  • recording the function declarations and if that symbol is exposed
  • recording the ambiguous function calls with what they can resolve too
  • there is two passes to know the aliases that need to be created:
    • checking the function declarations, to expose the functions that are configured as exposed
    • checking the ambiguous function calls, which define the necessary aliases

As shown in the last case with the non-ambiguous call to a polyfilled symbol inPrefix\FQ, not exposing the global constants would break. Unfortunately the only viable solution would be to require a second pass knowing in advance the polyfilled symbols. This is not possible as this requires not a second traverse of a file, but a second traverse of the codebase. Maybe possible but much slower and probably the API will be more awkward especially for Box.

Maybe another solution is to treat expose global functions differently, and expose them only if a usage is found. Either that or add a separate setting for it.

@theofidry theofidry added this to the 1.0.0 milestone Apr 28, 2024
@theofidry
Copy link
Member Author

Checking this issue again, it may not be as big of a deal as I make it out to be. Currently there is a lot of exposed symbols due to polyfills shipped, but in a way it should be fine as a polyfill is there to provide a very specific, specified PHP symbol. As such they should all behave the same so it should not be a problem.

For this reason I'll deprioritize this.

@theofidry theofidry removed this from the 1.0.0 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant