This feeds namespace information into Diviner's context fields, when it's available.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T2284: Make Diviner namespace aware
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- php_ns_diviner
- Lint
Lint Skipped - Unit
No Test Coverage - Build Status
Buildable 1479 Build 1479: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
| src/applications/diviner/atomizer/DivinerPHPAtomizer.php | ||
|---|---|---|
| 145 | I'm not sure this is ideal: it sets a method's context to the namespace without the class. It should probably either remain null or include both. The latter may make linking a bit more consistent (e.g. we have a ton of create static methods which could be disambiguated) | |
Comment Actions
I think I caught one material issue. I think it would be reasonable to detect + throw on that if you don't want to deal with getting it right, since it's pretty sketchy to write namespace X; multiple times in a singe source file.
I think we want to drop the trailing \\, but let me poke at D9791 and see if we run into anything awful.
| src/applications/diviner/atomizer/DivinerPHPAtomizer.php | ||
|---|---|---|
| 34–38 | Does this get the right result on: namespace Foo;
namespace Bar;
class A {}It looks like it will atomize it all as belonging to Foo. (This might be a bit tricky to get right.) | |
| 145 | Yeah, I would expect this to include the namespace and the class. | |
| src/applications/diviner/atomizer/DivinerPHPAtomizer.php | ||
|---|---|---|
| 34–38 | Woah - I'd always assumed that was invalid syntax, but sure enough it isn't. Shouldn't be too terrible to handle once I figure out what that actually does. | |
Comment Actions
Perform some crazy slizing and dicing to handle a format that PHP never should have supported:
<?php namespace Foo; class Foo { } class Bar { } namespace Bar; class Foo { } class Bar { }
Comment Actions
Just discovered that this screws up the line numbers from the source file since it rebuilds the tree during parsing. This feels like an incredibly wrong way to select, effectively, a few branches out of the AASTTree, but I couldn't find anything better in the source to handle it.
Comment Actions
Significant cleanup to NS range detection for weird files that do evil things. Fixes line numbering issue, and now includes class name in the context for method declarations
Comment Actions
- further clean up namespace/context work
- add php atomizer test cases and fix an issue they exposed
Comment Actions
This looks great to me, but I think I came up with a "clever" construction it gets wrong.
| src/applications/diviner/atomizer/DivinerPHPAtomizer.php | ||
|---|---|---|
| 32 | (Double semicolon.) | |
| 36–37 | Does this get: namespace X {
class A;
}
class B;...wrong? (never analyzes the global namespace at the end)? Or, for that matter, does it get this right? (misses the hole in the middle?) namespace X {
class A;
}
class B;
namespace Y {
class C;
}At a minimum, they don't seem to be represented in the test cases. (I'm also 100% fine with "detect this file is more crazy than anything you'll ever do and throw" at any level of complexity here.) | |
Comment Actions
My word, why would they allow syntax like that? I'll write a test case for it and see what happens, but that's probably just "give up you damn fool" territory :)
Comment Actions
Yeah, presumably there's not much intersection between the set of users who use namespaces like that and the set of users who want to generate codebase documentation.
Comment Actions
Turns out this is actually illegal (as I thought):
10:50:12 eric@Eric-MBP ~/dev/phabricator/phabricator/src/applications/diviner/atomizer/__tests__/data [0]$ php -l NamespaceMixed.phps
Fatal error: Cannot mix bracketed namespace declarations with unbracketed namespace declarations in NamespaceMixed.phps on line 7
Errors parsing NamespaceMixed.phps
10:50:16 eric@Eric-MBP ~/dev/phabricator/phabricator/src/applications/diviner/atomizer/__tests__/data [255]$ cat NamespaceMixed.phps
<?php namespace X { class A { } } namespace Y; class A { }