Use behavior thoughts

Hello!

2015-03-05 22:40 GMT+03:00 Anthony Ferrara ircmaxell@gmail.com:

This requires use to immediately follow namespace declarations:

namespace Foo {
use Bar; //valid
}
namespace Bar {
use Foo; //valid, second namespace in file
}
namespace Baz {
echo "Hi!";
use Foo; // Invalid
}

This did break approximately 30 internal tests, but they were all
testing this specific code path (or bugs related to it).

I like this way, because it's more logical and readable. There aren't
unpredictable effects and misunderstanding with interpretation of source
code.

So, I created a PR to remove this error:
https://github.com/php/php-src/pull/1149 https://github.com/php/php-src/pull/1149

+1

Note that there is no BC break here, as it's removing an error condition today.

This results in a weird edge case (which is 100% valid, but feels odd):

<?php
class Test {}
use Bar\Test;
var_dump(new Test); // class(Bar\Test)

That's perfectly valid, if not a bit weird.

The reverse would error mind you:

<?php
use Bar\Test;
class Test {}

Because you've already defined the symbol Test in the file.

To get around this, I've created another PR with a BC break, adding a
compile error if code comes before use:
https://github.com/php/php-src/pull/1150

-1, the example you gave looks totally valid.

Regards,
Mike

Hi Anthony.

This issue that has plagued me in the past, specifically with the use of
traits:

Error: http://3v4l.org/VFguK
OK: http://3v4l.org/73b86

Although when combined with opcache does causes very confusing behaviour, I
do worry that removing the error altogether may make it worse for
developers.

Take this example with constants:

namespace Biz {
const Bar = 41;
}

namespace Foo {
const Bar = 42;
}

namespace Foo {
use const Biz\Bar;
echo Bar;
}

// Fatal error: Cannot use const Biz\Bar as Bar because the name is already
in use

http://3v4l.org/0pRZk

An error here is far more useful that trying to exhibit the behaviour of
when the namespaces are declared in reverse order:

namespace Biz {
const Bar = 41;
}

namespace Foo {
use const Biz\Bar;
echo Bar;
}

namespace Foo {
const Bar = 42;
}

// 41

http://3v4l.org/E4PFX

I suppose the question here is; Does the declaration order matter?
Personally I think it should.

/@leedavis81

All,

Currently, use has a few error cases that are rather fragile. For example:

namespace Biz {
class Bar {};
}

namespace Foo {
use Biz\Bar;
$a = new Bar;
var_dump($a);
}

namespace Foo {
class Bar {};
}
http://3v4l.org/IPTiQ

That works 100% of the time, on 5.3.0 -> present.

But if we change the order of the two Foo blocks:

namespace Biz {
class Bar {};
}

namespace Foo {
class Bar {};
}

namespace Foo {
use Biz\Bar;
$a = new Bar;
var_dump($a);
}

http://3v4l.org/KlXUq

We get a Fatal error that "Cannot use Biz\Bar as Bar because the name
is already in use.

Basically, zend_compile.c is doing a check of the current symbol table
to see if the class is already defined when doing the use.

This is problematic not just for the example above, but when combined
with opcache. Opcache nulls out the symbol tables for file
compilation. So that means that file inclusion order matters when it's
disabled, but not when it's enabled.

This was discovered last night by Drupal 8 developers, when they
noticed that some developers claimed fatal errors, while they weren't
able to reproduce. After some debugging, it turned out that disabling
opcache caused the fatal errors. It was because of this symbol table
change.

The error is fragile. And IMHO it's not really protecting anything
significant, since it's clear within the file what each symbol refers
to (you need to look at use declarations for that anyway).

So, I created a PR to remove this error:
https://github.com/php/php-src/pull/1149

Note that there is no BC break here, as it's removing an error condition
today.

This results in a weird edge case (which is 100% valid, but feels odd):

<?php
class Test {}
use Bar\Test;
var_dump(new Test); // class(Bar\Test)

That's perfectly valid, if not a bit weird.

The reverse would error mind you:

<?php
use Bar\Test;
class Test {}

Because you've already defined the symbol Test in the file.

To get around this, I've created another PR with a BC break, adding a
compile error if code comes before use:
https://github.com/php/php-src/pull/1150

This requires use to immediately follow namespace declarations:

namespace Foo {
use Bar; //valid
}
namespace Bar {
use Foo; //valid, second namespace in file
}
namespace Baz {
echo "Hi!";
use Foo; // Invalid
}

This did break approximately 30 internal tests, but they were all
testing this specific code path (or bugs related to it).

What are your thoughts about both PRs? The first seems like a clear
win, the second is a nice cleanup but also a bit of a change to
behavior... I'm not sure if an RFC is required for either, but if
people want one (for both, for one, etc) I can draft something up
quickly.

Remove symbol check: https://github.com/php/php-src/pull/1149
Pin use to top: https://github.com/php/php-src/pull/1150

Thanks

Anthony