clang: Add a tool for MessageLoop refactoring
This tool performs refactoring on clients of MessageLoop and related APIs as described here:
https://docs.google.com/a/chromium.org/document/d/1qxdh2I61_aB_Uzh1QgNqvdWFBCL_E65G2smoSySw7KU/edit?pli=1#
Note that after running the tool, you should use the following script to fix includes and other things Clang doesn't know about:
$ tools/clang/refactor_message_loop/update_task_runner_headers.sh
BUG=465354
Hey Hans, I'm a total n00b at this so any pointers are welcome :) In ...
5 years, 9 months ago
(2015-03-19 20:10:10 UTC)
#2
Hey Hans, I'm a total n00b at this so any pointers are welcome :) In particular
I can't seem to figure out why the scoped_refptr<> matcher only finds things
inside the base namespace but not out of it.
On 2015/03/19 20:10:10, Sami wrote: > Hey Hans, I'm a total n00b at this so ...
5 years, 9 months ago
(2015-03-19 21:37:28 UTC)
#4
On 2015/03/19 20:10:10, Sami wrote:
> Hey Hans, I'm a total n00b at this so any pointers are welcome :) In
particular
> I can't seem to figure out why the scoped_refptr<> matcher only finds things
> inside the base namespace but not out of it.
+dcheng who knows the tooling. I've actually never used the AST matchers myself.
204 // Matches scoped_refptr<MessageLoopProxy>.
205 auto refptr_proxy_variable_matcher =
206 varDecl(hasType(recordDecl(hasName("::scoped_refptr"))),
207
hasDescendant(templateSpecializationType(hasAnyTemplateArgument(
208 refersToType(asString("class
base::MessageLoopProxy"))))))
209 .bind("var");
Wild guess, does it help if you remove the :: before scoped_refptr?
dcheng
On 2015/03/19 at 21:37:28, hans wrote: > On 2015/03/19 20:10:10, Sami wrote: > > Hey ...
5 years, 9 months ago
(2015-03-19 23:44:48 UTC)
#5
On 2015/03/19 at 21:37:28, hans wrote:
> On 2015/03/19 20:10:10, Sami wrote:
> > Hey Hans, I'm a total n00b at this so any pointers are welcome :) In
particular
> > I can't seem to figure out why the scoped_refptr<> matcher only finds things
> > inside the base namespace but not out of it.
>
> +dcheng who knows the tooling. I've actually never used the AST matchers
myself.
>
> 204 // Matches scoped_refptr<MessageLoopProxy>.
> 205 auto refptr_proxy_variable_matcher =
> 206 varDecl(hasType(recordDecl(hasName("::scoped_refptr"))),
> 207
hasDescendant(templateSpecializationType(hasAnyTemplateArgument(
> 208 refersToType(asString("class
base::MessageLoopProxy"))))))
> 209 .bind("var");
>
> Wild guess, does it help if you remove the :: before scoped_refptr?
In general, matching the AST is half art and science.
Off the top of my head, I don't know why you'd be seeing this issue. There are
several things you can do to help debug:
- Make liberal use of clang::Stmt::dump() and its variants.
- clang++ -cc1 -ast-dump <source file> <options (you can get this from the
compile DB)> to dump out the AST of an entire file.
- clang-query
(http://eli.thegreenplace.net/2014/07/29/ast-matchers-and-clang-refactoring-tools
has some documentation) to interactively try out your matcher (warning: this is
sometimes missing some matchers and/or has slightly different behavior. But it's
very useful as a starting point)
dcheng
On 2015/03/19 at 23:44:48, dcheng wrote: > On 2015/03/19 at 21:37:28, hans wrote: > > ...
5 years, 9 months ago
(2015-03-20 02:09:06 UTC)
#6
On 2015/03/19 at 23:44:48, dcheng wrote:
> On 2015/03/19 at 21:37:28, hans wrote:
> > On 2015/03/19 20:10:10, Sami wrote:
> > > Hey Hans, I'm a total n00b at this so any pointers are welcome :) In
particular
> > > I can't seem to figure out why the scoped_refptr<> matcher only finds
things
> > > inside the base namespace but not out of it.
> >
> > +dcheng who knows the tooling. I've actually never used the AST matchers
myself.
> >
> > 204 // Matches scoped_refptr<MessageLoopProxy>.
> > 205 auto refptr_proxy_variable_matcher =
> > 206 varDecl(hasType(recordDecl(hasName("::scoped_refptr"))),
> > 207
hasDescendant(templateSpecializationType(hasAnyTemplateArgument(
> > 208 refersToType(asString("class
base::MessageLoopProxy"))))))
> > 209 .bind("var");
> >
> > Wild guess, does it help if you remove the :: before scoped_refptr?
>
> In general, matching the AST is half art and science.
>
> Off the top of my head, I don't know why you'd be seeing this issue. There are
several things you can do to help debug:
> - Make liberal use of clang::Stmt::dump() and its variants.
> - clang++ -cc1 -ast-dump <source file> <options (you can get this from the
compile DB)> to dump out the AST of an entire file.
> - clang-query
(http://eli.thegreenplace.net/2014/07/29/ast-matchers-and-clang-refactoring-tools
has some documentation) to interactively try out your matcher (warning: this is
sometimes missing some matchers and/or has slightly different behavior. But it's
very useful as a starting point)
Note: you'll need to build clang-tools-extra for clang-query. There used to be
an option for this in the update.sh script, but I removed it in
https://codereview.chromium.org/17329002 when clang-format moved to the main
repository.
Sami
Thanks for the pointers! Would either of you be willing to rubberstamp this patch? It's ...
5 years, 7 months ago
(2015-05-06 17:25:33 UTC)
#7
Thanks for the pointers!
Would either of you be willing to rubberstamp this patch? It's not the prettiest
code, but has served me well in a couple of refactorings so far [1] [2] and
might be a useful reference for anyone who makes the same mistake of thinking
that it's simple to do this sort of stuff with clang :)
[1] https://codereview.chromium.org/1100773004
[2] https://codereview.chromium.org/1129903002
dcheng
(Sorry, apparently I had a bunch of old drafts I never sent out. I don't ...
5 years, 7 months ago
(2015-05-06 17:31:18 UTC)
#8
Thanks! All comments addressed. https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_message_loop/RefactorMessageLoop.cpp File tools/clang/refactor_message_loop/RefactorMessageLoop.cpp (right): https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_message_loop/RefactorMessageLoop.cpp#newcode10 tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:10: // => base::MessageLoop::task_runner()->PostTask*() On 2015/05/06 ...
5 years, 7 months ago
(2015-05-07 10:37:37 UTC)
#9
Thanks! All comments addressed.
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/RefactorMessageLoop.cpp (right):
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:10: // =>
base::MessageLoop::task_runner()->PostTask*()
On 2015/05/06 17:31:18, dcheng wrote:
> Dumb question: why not just update MessageLoop::PostTask to use task_runner()
> internally? It seems like we're just adding more typing.
There is some more discussion on the linked design doc and bug, but the tl;dr is
that we can make code more modular by not having it depend on the concrete
base::MessageLoop but rather a more abstract task runner.
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:76: std::string
ReplaceFirst(const std::string& input,
On 2015/05/06 17:31:18, dcheng wrote:
> Consider using llvm::StringRef where appropriate, which is roughly equivalent
to
> StringPiece.
Neat, thanks for the tip. Done.
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:88:
PostTaskCallback(Replacements* replacements) : replacements_(replacements) {}
On 2015/05/06 17:31:18, dcheng wrote:
> explicit
Fixed (here and elsewhere).
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:90: virtual void
run(const MatchFinder::MatchResult& result) override;
On 2015/05/06 17:31:17, dcheng wrote:
> No virtual.
Fixed (here and elsewhere).
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:241: obj_text +
(is_arrow ? "->" : ".") + "task_runner()->" + method;
On 2015/05/06 17:31:17, dcheng wrote:
> Isn't this always -> in Chrome?
That's almost always the case, but some tests do have an explicit MessageLoop
instance on the stack, e.g:
MessageLoop loop;
loop.PostTask(...);
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:243: // Rewrite
MessageLoop::current() as ThreadTaskRunnerHandle::Get().
On 2015/05/06 17:31:18, dcheng wrote:
> Why is this chunk here? I would have expected to see this in
> CurrentProxyCallback below.
CurrentProxyCallback matches calls to MessageLoopProxy::current(), but this one
looks for calls like MessageLoop::current()->PostTask(...). That is, they're
operating on different classes (MessageLoopProxy vs. MessageLoop).
https://codereview.chromium.org/1010073002/diff/180001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:265: if
(text.find("base::") == 0) {
On 2015/05/06 17:31:18, dcheng wrote:
> Hmm... I guess I should finish my patch to try to figure out the minimum
> namespace qualifiers needed for a symbol...
Got a link? Sounds useful :)
Although for this particular rewrite pretty much everyone outside base/ needs
the namespace qualifier so I wouldn't imagine being smarter here would help a
lot.
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/RefactorMessageLoop.cpp (right):
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:589: if (prev_end >
r.getOffset())
On 2015/05/06 17:31:18, dcheng wrote:
> Are you actually generating overlapping edits, or identical ones?
They are identical in the sense that the end result is the same but different
parts of the edit can be generated by different rules.
IIRC for example in some contexts "scoped_refptr<base::MessageLoopProxy> proxy"
is first rewritten to "scoped_refptr<base::SingleThreadTaskRunner> task_runner"
and then a further rule would try to rewrite "proxy" into "task_runner" but
would fail because the first edit changed the offset.
Maybe a cleaner way to do that would be to split this into two tools -- one for
rewriting classes and another for rewriting variables.
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/scripts/ru...
File tools/clang/scripts/run_tool.py (right):
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/scripts/ru...
tools/clang/scripts/run_tool.py:321: extensions = frozenset(('.c', '.cc', '.h',
'.m', '.mm'))
On 2015/05/06 17:31:18, dcheng wrote:
> Why does this need to change? There (should) never be header files as compile
> rules in the compile DB.
IIRC without this some .h files were never getting rewritten. I'll take this out
for now and put together another CL if I run into this again.
5 years, 7 months ago
(2015-05-07 13:31:35 UTC)
#10
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/scripts/ru...
File tools/clang/scripts/run_tool.py (right):
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/scripts/ru...
tools/clang/scripts/run_tool.py:321: extensions = frozenset(('.c', '.cc', '.h',
'.m', '.mm'))
On 2015/05/07 10:37:37, Sami wrote:
> On 2015/05/06 17:31:18, dcheng wrote:
> > Why does this need to change? There (should) never be header files as
compile
> > rules in the compile DB.
>
> IIRC without this some .h files were never getting rewritten. I'll take this
out
> for now and put together another CL if I run into this again.
Yeah, turns out if I don't do this, none of the .h files get edited. I think
it's because _ApplyEdits below limits the edits to files to with matching
extensions.
Sami
Friendly ping?
5 years, 7 months ago
(2015-05-12 17:19:50 UTC)
#11
Friendly ping?
dcheng
Sorry I'm in Sydney at BlinkOn atm, so just one quick thought. https://codereview.chromium.org/1010073002/diff/480001/tools/clang/scripts/run_tool.py File tools/clang/scripts/run_tool.py ...
5 years, 7 months ago
(2015-05-13 05:07:05 UTC)
#12
On 2015/05/13 05:07:05, dcheng wrote: > Sorry I'm in Sydney at BlinkOn atm, so just ...
5 years, 7 months ago
(2015-05-13 18:33:36 UTC)
#13
On 2015/05/13 05:07:05, dcheng wrote:
> Sorry I'm in Sydney at BlinkOn atm, so just one quick thought.
Ah, I didn't realize you were travelling. No rush, have a good trip :)
https://codereview.chromium.org/1010073002/diff/480001/tools/clang/scripts/ru...
File tools/clang/scripts/run_tool.py (right):
https://codereview.chromium.org/1010073002/diff/480001/tools/clang/scripts/ru...
tools/clang/scripts/run_tool.py:321: extensions = frozenset(('.c', '.cc', '.h',
'.m', '.mm'))
On 2015/05/13 05:07:05, dcheng wrote:
> I think the right thing to do is change the set of filenames that
_ApplyEdits()
> operates on below. It doesn't make sense to pass .h files to the compiler
> dispatcher.
Yes, that's much cleaner. Done.
Sami
Ready for another pass on this one?
5 years, 6 months ago
(2015-05-26 20:23:28 UTC)
#14
Ready for another pass on this one?
dcheng
Sorry, I have some comments queued but forgot to send. As mentioned in PS27 though, ...
5 years, 6 months ago
(2015-05-26 20:35:44 UTC)
#15
Sorry, I have some comments queued but forgot to send.
As mentioned in PS27 though, my primary concern is that while the tool works, it
doesn't illustrate best practices in terms of writing a tool for refactoring. In
many places, the tool overmatches, grabs the source text of the entire match,
and then tries to use string replacement to hopefully do the right thing. In
many of these cases, the clang AST should be providing enough source location
information that we can precisely match only what needs to be changed.
For example, in CustomProxyGetterCallback, instead of grabbing the entire source
text for the CXXMethodDecl:
"MessageLoopProxy* SomeMethodThatReturnsAMLP() { /* ... */ }"
and then using string find-and-replace to change that to
"SingleThreadTaskRunner* SomeMethodThatReturnsAMLP() { /* ... */ }"
we should be using FunctionDecl::getReturnTypeSourceRange() and emitting a
precise replacement. A similar comment applies to the other MatchCallback
implementations.
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/RefactorMessageLoop.cpp (right):
https://codereview.chromium.org/1010073002/diff/420001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:589: if (prev_end >
r.getOffset())
On 2015/05/07 10:37:37, Sami wrote:
> On 2015/05/06 17:31:18, dcheng wrote:
> > Are you actually generating overlapping edits, or identical ones?
>
> They are identical in the sense that the end result is the same but different
> parts of the edit can be generated by different rules.
>
> IIRC for example in some contexts "scoped_refptr<base::MessageLoopProxy>
proxy"
> is first rewritten to "scoped_refptr<base::SingleThreadTaskRunner>
task_runner"
> and then a further rule would try to rewrite "proxy" into "task_runner" but
> would fail because the first edit changed the offset.
>
> Maybe a cleaner way to do that would be to split this into two tools -- one
for
> rewriting classes and another for rewriting variables.
I'm really surprised that you need to do this. The rewrite that you're
describing should be OK: _ApplyEdits() in the script tries to apply the edits in
reverse order. So proxy should be rewritten to task_runner first, and then the
scoped_refptr<> parameter should get rewritten after that. This should prevent
edits from conflicting...
If this is a bug in run_tools.py, it seems like we should try to fix that.
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/CMakeLists.txt (right):
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/CMakeLists.txt:8: RefactorMessageLoop.cpp
Nit: two spaces
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/RefactorMessageLoop.cpp (right):
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:107: std::string
RenameMessageLoopProxyVariable(std::string name) {
Is this problematic at all in practice? We're replacing a VarDecl (or FieldDecl,
or some other Decl) without necessarily updating all the other DeclRefExprs that
might point to it, right?
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:305: auto
refptr_proxy_field_matcher =
Does something like recordDecl(hasName("::scoped_refptr"),
templateSpecializationType(hasTemplateArgument(0,
refersToDeclaration(is_message_loop_proxy)))) work?
Since we want to commit this, it'd be nice to try to figure out the right way to
hook up the AST matchers, rather than trying to match the type in each callback.
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:321: // couldn't come
up with a reliable AST matcher for doing that here.
By reliable, do you mean it worked sometimes and not others? Or it just didn't
work at all?
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:497: const
DeclRefExpr* ref = result.Nodes.getNodeAs<DeclRefExpr>("ref");
Seems to me that the code would be simpler if you just bound these to "expr" as
well: you can get rid of the nested ternaries on line 500, as well as
significantly simplifying the if block after that. The only real difference
seems like it's for CXXCtorInitializer* anyway
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:531: // conflict the
edit that replaces the getter.
It shouldn't, as long as you're not clobbering it by rewriting the content
inside the ()s as well. We can do this by manually building a replacement source
range from the ctor->getSourceLocation() to ctor->getLParenLoc().
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/tests/test-original.cc (right):
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/tests/test-original.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
In the future, I would recommend trying to split up the tests a bit. I didn't
really look through here, and I just kind of assume it happens to cover all the
interesting cases.
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/update_task_runner_headers.sh (right):
https://codereview.chromium.org/1010073002/diff/500001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/update_task_runner_headers.sh:1: #!/bin/sh
How will you handle this for Windows? Generate the edits on Windows, and then
run the fixup script later on Linux?
https://codereview.chromium.org/1010073002/diff/520001/tools/clang/refactor_m...
File tools/clang/refactor_message_loop/RefactorMessageLoop.cpp (right):
https://codereview.chromium.org/1010073002/diff/520001/tools/clang/refactor_m...
tools/clang/refactor_message_loop/RefactorMessageLoop.cpp:422: std::string
replacement = ns + "ThreadTaskRunnerHandle::Get()";
My main objection to landing this patch as-is is that we're not really using the
full information from the clang AST. A lot of these things, we could get precise
source ranges for and emit replacements for. This tool elects to do a
find-string-and-replace once it's "close enough" though.
If we do want to check this in as a 'best practices' kind of tool, I would
prefer to see us be much more precise in emitting replacements. This would
eliminate the need for "ReplaceFirst" as well as manually trying to figure out
the right namespace qualifiers.
https://codereview.chromium.org/1010073002/diff/520001/tools/clang/scripts/ru...
File tools/clang/scripts/run_tool.py (right):
https://codereview.chromium.org/1010073002/diff/520001/tools/clang/scripts/ru...
tools/clang/scripts/run_tool.py:336: if os.path.realpath(k) in
editable_filenames},
I think it's OK to just have this be "all_filenames". Then we don't need
"editable_filenames" at all.
dcheng
(Also, if you want to land the run_tool.py changes separately, that'd be fine with me)
5 years, 6 months ago
(2015-05-26 20:36:11 UTC)
#16
(Also, if you want to land the run_tool.py changes separately, that'd be fine
with me)
Issue 1010073002: clang: Add a tool for MessageLoop refactoring
(Closed)
Created 5 years, 9 months ago by Sami
Modified 4 years, 10 months ago
Reviewers: dcheng, hans
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 32