Don't walk inheritance chain of return type to make renaming decisions.

Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

Known cases that 1) need a "Get" prefix and 2) would have regressed
after this CL are 3) covered by adding them to the hardcoded list in
ShouldPrefixFunctionName.

BUG=673031
Committed: https://crrev.com/004f407089920c3d104643c7a20b49bc14ad8fd2
Cr-Commit-Position: refs/heads/master@{#440791}

[Łukasz Anforowicz, 2016-12-23 00:56:38]

Description was changed from

==========
Don't walking inheritance chain of return type to make renaming decisions.

Walking inheritance chain of a type depends too much on the context to
be a reliable renaming decision signal.  For example - walking the
inheritance chain might not be possible if a class is forward declared.
Consequently, walking the inheritance chain can lead to making two
different renaming decisions for the same file location (the interesting
"Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to consistently rename the method name in base and derived
classes.  This is what this CL switches to.

BUG=673031
==========

to

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

BUG=673031
==========

[Łukasz Anforowicz, 2016-12-23 00:56:54]

Description was changed from

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

BUG=673031
==========

to

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

BUG=673031
==========

[Łukasz Anforowicz, 2016-12-23 01:01:36]

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

[Łukasz Anforowicz, 2016-12-23 01:01:37]

Daniel, can you PTAL?  I believe that this CL should fix the "Framame" bug (the
fix is not verified, but I feel pretty confident that the issues pointed in the
CL description is real and should be addressed).

[Łukasz Anforowicz, 2016-12-23 01:10:35]

Description was changed from

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

BUG=673031
==========

to

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

Known cases that 1) need a "Get" prefix and 2) would have regressed
after this CL are 3) covered by adding them to the hardcoded list in
ShouldPrefixFunctionName.

BUG=673031
==========

[dcheng, 2016-12-23 05:52:11]

One question and one alternative proposal:
Since we're splitting up edit generation from edit application, we could resolve
conflicts prior to edit application. If we ever see two rewrites to the same
location, one adding a Get prefix, and the other not, then we should upgrade all
the rewrites to use the Get prefix. WDYT?

https://codereview.chromium.org/2601513005/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc (right):

https://codereview.chromium.org/2601513005/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc:231:
LayoutBoxModelObject* LayoutObject() { return nullptr; }
Isn't this rename going to be problematic?

I see it in the "force prefix" list, so I'm wondering why it's not prefixed with
Get.

[Łukasz Anforowicz, 2016-12-27 19:36:10]

On 2016/12/23 05:52:11, dcheng wrote:
> Since we're splitting up edit generation from edit application, we could
resolve
> conflicts prior to edit application. If we ever see two rewrites to the same
> location, one adding a Get prefix, and the other not, then we should upgrade
all
> the rewrites to use the Get prefix. WDYT?

I think this cannot work.  Consider the example below:

definition_of_child.h:
    class Base {};
    class Child : public Base {};

foo.h:
    class Child;  // forward decl - no idea that Child derives from Base.
    class Foo {
      Child* base() { return nullptr; }
    };

x.cpp:
    // Will rename |base| -> |GetBase| in accessor.h and below.
    #include "definition_of_child.h"
    #include "foo.h"
    void x() {
      Foo foo;
      void* p = foo.base();
    }

y.cpp:
    // Will rename |base| -> |Base| in accessor.h and below.

    // No idea that Child derives from Base without the include below:
    // #include "definition_of_child.h"
    #include "foo.h"

    void y() {
      Foo foo;
      void* p = foo.base();
    }

In the example above:
1. We *consistently* rename |base| to |GetBase| in body of x.cpp / in body of
x()
2. We *consistently* rename |base| to |Base| in body of y.cpp / in body of y()
3. We *inconsistently* rename |base| to |GetBase| or |Base| in foo.h

We cannot remove inconsistencies locally (i.e. in 3 above) without considering
global inconsistencies (i.e. 1 vs 2).



I think the CL under review will work, because it will always produce consistent
set of renames (because the renames are based on information [decl of method
being overriden [if any]] that I believe is guaranteed to be always present when
analysing the decl of a method).

https://codereview.chromium.org/2601513005/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc (right):

https://codereview.chromium.org/2601513005/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc:231:
LayoutBoxModelObject* LayoutObject() { return nullptr; }
On 2016/12/23 05:52:11, dcheng wrote:
> Isn't this rename going to be problematic?
> 
> I see it in the "force prefix" list, so I'm wondering why it's not prefixed
with
> Get.

Ooops.  It was prefixed with "Get" and the tests were failing - I didn't rerun
the tests after adding "layoutObject" to the hardcoded list of names in
ShouldPrefixFunctionName.

I've updated the test here to only deal with inheritance.  Tests are passing
now.

[dcheng, 2016-12-27 20:41:50]

On 2016/12/27 19:36:10, Łukasz Anforowicz wrote:
> On 2016/12/23 05:52:11, dcheng wrote:
> > Since we're splitting up edit generation from edit application, we could
> resolve
> > conflicts prior to edit application. If we ever see two rewrites to the same
> > location, one adding a Get prefix, and the other not, then we should upgrade
> all
> > the rewrites to use the Get prefix. WDYT?
> 
> I think this cannot work.  Consider the example below:
> 
> definition_of_child.h:
>     class Base {};
>     class Child : public Base {};
> 
> foo.h:
>     class Child;  // forward decl - no idea that Child derives from Base.
>     class Foo {
>       Child* base() { return nullptr; }
>     };
> 
> x.cpp:
>     // Will rename |base| -> |GetBase| in accessor.h and below.
>     #include "definition_of_child.h"
>     #include "foo.h"
>     void x() {
>       Foo foo;
>       void* p = foo.base();
>     }
> 
> y.cpp:
>     // Will rename |base| -> |Base| in accessor.h and below.
> 
>     // No idea that Child derives from Base without the include below:
>     // #include "definition_of_child.h"
>     #include "foo.h"
> 
>     void y() {
>       Foo foo;
>       void* p = foo.base();
>     }
> 
> In the example above:
> 1. We *consistently* rename |base| to |GetBase| in body of x.cpp / in body of
> x()
> 2. We *consistently* rename |base| to |Base| in body of y.cpp / in body of y()
> 3. We *inconsistently* rename |base| to |GetBase| or |Base| in foo.h
> 
> We cannot remove inconsistencies locally (i.e. in 3 above) without considering
> global inconsistencies (i.e. 1 vs 2).

Right, we would need to resolve this globally.

In general, I don't think we forward declare functions too much in Blink, while
you can't really forward declare a method. So we could use the referenced
declaration as a key, and rewrites touching functions would point to the
declaration.

Before the extract_edits step, we would perform global conflict resolution for
functions: if any reference to a declared function is rewritten to use Get, all
references must be upgraded to use Get, etc. This is, admittedly, more complex.

> 
> 
> 
> I think the CL under review will work, because it will always produce
consistent
> set of renames (because the renames are based on information [decl of method
> being overriden [if any]] that I believe is guaranteed to be always present
when
> analysing the decl of a method).
> 
>
https://codereview.chromium.org/2601513005/diff/20001/tools/clang/rewrite_to_...
> File tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc (right):
> 
>
https://codereview.chromium.org/2601513005/diff/20001/tools/clang/rewrite_to_...
> tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc:231:
> LayoutBoxModelObject* LayoutObject() { return nullptr; }
> On 2016/12/23 05:52:11, dcheng wrote:
> > Isn't this rename going to be problematic?
> > 
> > I see it in the "force prefix" list, so I'm wondering why it's not prefixed
> with
> > Get.
> 
> Ooops.  It was prefixed with "Get" and the tests were failing - I didn't rerun
> the tests after adding "layoutObject" to the hardcoded list of names in
> ShouldPrefixFunctionName.
> 
> I've updated the test here to only deal with inheritance.  Tests are passing
> now.

If this resolves the remaining errors though, this LGTM as a simpler fix.

[Łukasz Anforowicz, 2016-12-27 21:26:28]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-12-27 21:26:38]

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-27 21:36:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482873988950300,
"parent_rev": "229bc0d9bc7b947e19cf0904e2e0d710f81b3ae6", "commit_rev":
"3bac61e1436f4125f2fec4622dbc5157fd865553"}

[commit-bot: I haz the power, 2016-12-27 21:36:40]

Description was changed from

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

Known cases that 1) need a "Get" prefix and 2) would have regressed
after this CL are 3) covered by adding them to the hardcoded list in
ShouldPrefixFunctionName.

BUG=673031
==========

to

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

Known cases that 1) need a "Get" prefix and 2) would have regressed
after this CL are 3) covered by adding them to the hardcoded list in
ShouldPrefixFunctionName.

BUG=673031

Review-Url: https://codereview.chromium.org/2601513005
==========

[commit-bot: I haz the power, 2016-12-27 21:36:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2017-01-02 15:47:21]

Description was changed from

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

Known cases that 1) need a "Get" prefix and 2) would have regressed
after this CL are 3) covered by adding them to the hardcoded list in
ShouldPrefixFunctionName.

BUG=673031

Review-Url: https://codereview.chromium.org/2601513005
==========

to

==========
Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

Known cases that 1) need a "Get" prefix and 2) would have regressed
after this CL are 3) covered by adding them to the hardcoded list in
ShouldPrefixFunctionName.

BUG=673031

Committed: https://crrev.com/004f407089920c3d104643c7a20b49bc14ad8fd2
Cr-Commit-Position: refs/heads/master@{#440791}
==========

[commit-bot: I haz the power, 2017-01-02 15:47:22]

Patchset 3 (id:??) landed as
https://crrev.com/004f407089920c3d104643c7a20b49bc14ad8fd2
Cr-Commit-Position: refs/heads/master@{#440791}