Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr nodes.

Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr nodes.

BUG=582315
Committed: https://crrev.com/1e1f3d0f63447b5c8c2fbccf7f37e39b70305961
Cr-Commit-Position: refs/heads/master@{#435654}

[Łukasz Anforowicz, 2016-08-17 22:45:08]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2016-08-17 22:45:09]

Daniel, can you take a look please?

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (left):

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:441: static const
char* GetType() { return "NamedDecl"; }
Deleting - this trait was unused AFAICT.

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:450: }
Not sure if I can/should handle other kinds of renames (e.g. UPPER_CASE ... ->
kUpperCase?).

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:549: }
ContainsIdentifier, IsDeclarationMacroTokenConcatenated, GetCurrentName
functions were introduced to allow reuse of RewriterBase for
DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr (otherwise
RewriterBase::run body would refer to members not present in
DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr: getIdentifier,
getName, etc.).

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:687:
allOf(hasAncestor(namespaceDecl(anyOf(hasName("blink"), hasName("WTF")),
s/decl/allOf/ because DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr
are not a "decl" (i.e. their matchers do not accept a matcher/filter limited to
decls only [like |in_blink_namespace| was before this change]).

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:942:
expr(id("decl", dependentScopeDeclRefExpr(in_blink_namespace)));
One could argue that I should say "expr" instead of "decl" above... But
RewriterBase::run hardcodes "decl"...  Path of least resistance... :-)  One
alternative would be to have a trait for GetTargetName (old GetName) and for
GetSourceName) - this seems like too much trouble IMO.

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/function-templates-expected.cc
(right):

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/function-templates-expected.cc:28:
value_ = static_cast<T>(rhs.value_);
Ha!  This is the "m_" heuristic in effect - seems to work :-)

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/template-expected.cc (right):

https://codereview.chromium.org/2256913002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/template-expected.cc:31: 
Test that DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr are not
rewritten if they are outside of blink namespaces.

[Łukasz Anforowicz, 2016-08-18 00:14:42]

Description was changed from

==========
Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr nodes.

BUG=584117
==========

to

==========
Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr nodes.

BUG=582315
==========

[Łukasz Anforowicz, 2016-08-23 20:49:39]

Daniel, can you take a look please?

I think this CL is now really ready for a review - after applying all
the 3 CLs below, and running the tool on Source/wtf, things start to
compile without any errors (so far I've tested building of PrintStream.o).
This increases my confidence that the 3 CLs below really are moving things
in the right direction:
- https://codereview.chromium.org/2246263002
- https://codereview.chromium.org/2274653002
- https://codereview.chromium.org/2256913002

Note that this CL exposes a bug in clang -
https://llvm.org/bugs/show_bug.cgi?id=29042.  I think the CL is ready for a
review despite this bug, but I am not sure if I can land the CL before this bug
is fixed.  For now I am (locally) working around the bug by simply returning
false from matchesAncestorOfRecursively in ASTMatchFinder.cpp if
|Parents.empty()|.

[Łukasz Anforowicz, 2016-08-25 18:11:14]

https://codereview.chromium.org/2256913002/diff/80001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1082:
anyOf(pointsTo(in_blink_namespace), references(in_blink_namespace),
This strips only 1 layer of indirection, but I don't know how to improve this. 
In another CR (*), your suggestion (to call QualType::getBaseTypeIdentifier())
helps, but here we need something like |getBaseType| (without the identifier
part).

(*)
https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...

https://codereview.chromium.org/2256913002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1088:
typedefType(hasDeclaration(in_blink_namespace))))));
I don't know how I can avoid listing all the various type flavors here...

[dcheng, 2016-11-30 05:55:53]

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:182: return
!base_expr || InnerMatcher.matches(*base_expr, Finder, Builder);
What's an implicit access look like? Why is it OK to always return a match in
that case?

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:192: return !qual
|| InnerMatcher.matches(*qual, Finder, Builder);
Similar question here: why is it OK to return a match if there is no qualifier?

Shouldn't the matcher expression that composes these just use anyOf() to select
as appropriate?

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:809: std::string
GuessNameForUnresolvedDependentNode(const TargetNode& node,
Can we keep this consistent with GetNameForDecl()?

[Łukasz Anforowicz, 2016-11-30 23:26:53]

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:182: return
!base_expr || InnerMatcher.matches(*base_expr, Finder, Builder);
On 2016/11/30 05:55:52, dcheng wrote:
> What's an implicit access look like? Why is it OK to always return a match in
> that case?

1. You are right that we should not return a match if Node.isImplicitAccess() is
true.


2. Consider the following case, which illustrates why combining hasBaseExpr...
and hasQualifier... via *anyOf* is undesirable:

    blinkObject->NonBlinkBaseClass::virtualMethod()  

In this case we *cannot* rename even though hasBaseExpr(blinkType(...)) matches
here, because the call is to a method from outside of blink - this will be
rejected because NonBlinkBaseClass won't match via
hasQualifierMissingOrMatching.


3. I hope I made things better by combining the 2 matchers into a single
matcher.  The new matcher is also more correct as well (the old code would
refuse to rename nonBlinkObject->BlinkClass::method()).  I've added a regression
test for this.

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:192: return !qual
|| InnerMatcher.matches(*qual, Finder, Builder);
On 2016/11/30 05:55:52, dcheng wrote:
> Similar question here: why is it OK to return a match if there is no
qualifier?
> 
> Shouldn't the matcher expression that composes these just use anyOf() to
select
> as appropriate?

See above.

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:809: std::string
GuessNameForUnresolvedDependentNode(const TargetNode& node,
On 2016/11/30 05:55:52, dcheng wrote:
> Can we keep this consistent with GetNameForDecl()?

Done (I think).

There is some duplication for calculating the |old_name| both in |run| and in
GuessNameForUnresolvedDependentNode methods, but I guess this is okay (and I
think I like how GuessNameForUnresolvedDependentNode doesn't need to take
|old_name| as a parameter).

[dcheng, 2016-12-01 07:15:06]

lgtm

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:182: return
!base_expr || InnerMatcher.matches(*base_expr, Finder, Builder);
On 2016/11/30 23:26:53, Łukasz Anforowicz wrote:
> On 2016/11/30 05:55:52, dcheng wrote:
> > What's an implicit access look like? Why is it OK to always return a match
in
> > that case?
> 
> 1. You are right that we should not return a match if Node.isImplicitAccess()
is
> true.
> 
> 
> 2. Consider the following case, which illustrates why combining hasBaseExpr...
> and hasQualifier... via *anyOf* is undesirable:
> 
>     blinkObject->NonBlinkBaseClass::virtualMethod()  
> 
> In this case we *cannot* rename even though hasBaseExpr(blinkType(...))
matches
> here, because the call is to a method from outside of blink - this will be
> rejected because NonBlinkBaseClass won't match via
> hasQualifierMissingOrMatching.
> 
> 
> 3. I hope I made things better by combining the 2 matchers into a single
> matcher.  The new matcher is also more correct as well (the old code would
> refuse to rename nonBlinkObject->BlinkClass::method()).  I've added a
regression
> test for this.

I see. I think the usual preference is to have smaller, more composable matchers
(we could still compose with anyOf, we just have to be insert an additional
unless() matcher to fix #2), but I can see the argument either way here.

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:809: std::string
GuessNameForUnresolvedDependentNode(const TargetNode& node,
On 2016/11/30 23:26:53, Łukasz Anforowicz wrote:
> On 2016/11/30 05:55:52, dcheng wrote:
> > Can we keep this consistent with GetNameForDecl()?
> 
> Done (I think).
> 
> There is some duplication for calculating the |old_name| both in |run| and in
> GuessNameForUnresolvedDependentNode methods, but I guess this is okay (and I
> think I like how GuessNameForUnresolvedDependentNode doesn't need to take
> |old_name| as a parameter).

I think I'm OK with passing old_name in; just having two different return styles
(one returns false, one returns empty string) is a bit confusing.

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:760: // Heuristic -
looking only at the shape of AST, let's assume that |using
Nit: perhaps refine this comment to say that from just the AST, it's impossible
to tell if a using statement is importing a callable or non-callable object.
Also mention that at this point, we've hopefully filtered out fields by checking
for m_, so we blindly assume that anything left is probably a callable. 

Something along those lines.

(Calling out the earlier m_ filtering is nice for documentation, since it's not
so obviously connected. Perhaps we could do that by just adding an assert() that
it doesn't start with m_ here?)

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/tests/template-expected.cc (right):

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/tests/template-expected.cc:265: namespace
cxx_dependend_scope_member_expr_testing {
Nit: dependent?

[Łukasz Anforowicz, 2016-12-01 18:26:57]

Thanks for the review.

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:182: return
!base_expr || InnerMatcher.matches(*base_expr, Finder, Builder);
On 2016/12/01 07:15:06, dcheng wrote:
> On 2016/11/30 23:26:53, Łukasz Anforowicz wrote:
> > On 2016/11/30 05:55:52, dcheng wrote:
> > > What's an implicit access look like? Why is it OK to always return a match
> in
> > > that case?
> > 
> > 1. You are right that we should not return a match if
Node.isImplicitAccess()
> is
> > true.
> > 
> > 
> > 2. Consider the following case, which illustrates why combining
hasBaseExpr...
> > and hasQualifier... via *anyOf* is undesirable:
> > 
> >     blinkObject->NonBlinkBaseClass::virtualMethod()  
> > 
> > In this case we *cannot* rename even though hasBaseExpr(blinkType(...))
> matches
> > here, because the call is to a method from outside of blink - this will be
> > rejected because NonBlinkBaseClass won't match via
> > hasQualifierMissingOrMatching.
> > 
> > 
> > 3. I hope I made things better by combining the 2 matchers into a single
> > matcher.  The new matcher is also more correct as well (the old code would
> > refuse to rename nonBlinkObject->BlinkClass::method()).  I've added a
> regression
> > test for this.
> 
> I see. I think the usual preference is to have smaller, more composable
matchers
> (we could still compose with anyOf, we just have to be insert an additional
> unless() matcher to fix #2), but I can see the argument either way here.

Acknowledged.

https://codereview.chromium.org/2256913002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:809: std::string
GuessNameForUnresolvedDependentNode(const TargetNode& node,
On 2016/12/01 07:15:06, dcheng wrote:
> On 2016/11/30 23:26:53, Łukasz Anforowicz wrote:
> > On 2016/11/30 05:55:52, dcheng wrote:
> > > Can we keep this consistent with GetNameForDecl()?
> > 
> > Done (I think).
> > 
> > There is some duplication for calculating the |old_name| both in |run| and
in
> > GuessNameForUnresolvedDependentNode methods, but I guess this is okay (and I
> > think I like how GuessNameForUnresolvedDependentNode doesn't need to take
> > |old_name| as a parameter).
> 
> I think I'm OK with passing old_name in; just having two different return
styles
> (one returns false, one returns empty string) is a bit confusing.

Done.

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:760: // Heuristic -
looking only at the shape of AST, let's assume that |using
On 2016/12/01 07:15:06, dcheng wrote:
> Nit: perhaps refine this comment to say that from just the AST, it's
impossible
> to tell if a using statement is importing a callable or non-callable object.
> Also mention that at this point, we've hopefully filtered out fields by
checking
> for m_, so we blindly assume that anything left is probably a callable. 
> 
> Something along those lines.
> 
> (Calling out the earlier m_ filtering is nice for documentation, since it's
not
> so obviously connected. Perhaps we could do that by just adding an assert()
that
> it doesn't start with m_ here?)

Done.

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/tests/template-expected.cc (right):

https://codereview.chromium.org/2256913002/diff/260001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/tests/template-expected.cc:265: namespace
cxx_dependend_scope_member_expr_testing {
On 2016/12/01 07:15:06, dcheng wrote:
> Nit: dependent?

Ooops.  Done. :-)

[Łukasz Anforowicz, 2016-12-01 18:27:07]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-12-01 18:27:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2256913002/#ps300001
(title: "Tweaked a comment + fixed a typo.")

[commit-bot: I haz the power, 2016-12-01 18:27:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 18:36:40]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1480616827091590,
"parent_rev": "2e31da41e3ea7e91579f83255150f7a00f151b0b", "commit_rev":
"b77ad1b3d47f29642938af4c2825467f6e0df017"}

[commit-bot: I haz the power, 2016-12-01 18:37:06]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-12-01 18:39:08]

Description was changed from

==========
Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr nodes.

BUG=582315
==========

to

==========
Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr nodes.

BUG=582315

Committed: https://crrev.com/1e1f3d0f63447b5c8c2fbccf7f37e39b70305961
Cr-Commit-Position: refs/heads/master@{#435654}
==========

[commit-bot: I haz the power, 2016-12-01 18:39:09]

Patchset 16 (id:??) landed as
https://crrev.com/1e1f3d0f63447b5c8c2fbccf7f37e39b70305961
Cr-Commit-Position: refs/heads/master@{#435654}