New issue
Advanced search Search tips

Issue 777575 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

clang plugin gets confused when inserting an override specifier for a defaulted function

Project Member Reported by dcheng@chromium.org, Oct 23 2017

Issue description

diff --git a/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h b/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h
index f96565d805cb..2e16aa27c84d 100644
--- a/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h
+++ b/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h
@@ -31,10 +31,10 @@ class CORE_EXPORT ScriptedIdleTaskController
   static ScriptedIdleTaskController* Create(ExecutionContext* context) {
     return new ScriptedIdleTaskController(context);
   }
-  ~ScriptedIdleTaskController();
+  ~ScriptedIdleTaskController() override;

-  void Trace(blink::Visitor*);
-  void TraceWrappers(const ScriptWrappableVisitor*) const;
+  void Trace(blink::Visitor*) override;
+  void TraceWrappers(const ScriptWrappableVisitor*) const override;

   using CallbackId = int;

@@ -44,7 +44,7 @@ class CORE_EXPORT ScriptedIdleTaskController
                    public TraceWrapperBase {
    public:
     virtual void Trace(blink::Visitor* visitor) {}
-    virtual void TraceWrappers(const ScriptWrappableVisitor* visitor) const {}
+    virtual void TraceWrappers(const ScriptWrappableVisitor* visitor) const  override{}
     virtual ~IdleTask() = default;
     virtual void invoke(IdleDeadline*) = 0;
   };
@@ -56,10 +56,10 @@ class CORE_EXPORT ScriptedIdleTaskController
     static V8IdleTask* Create(V8IdleRequestCallback* callback) {
       return new V8IdleTask(callback);
     }
-    ~V8IdleTask() = default;
+    ~V8IdleTask()  override= defaul overridet;
     void invoke(IdleDeadline*) override;
-    void Trace(blink::Visitor*);
-    void TraceWrappers(const ScriptWrappableVisitor*) const;
+    void Trace(blink::Visitor*) override;
+    void TraceWrappers(const ScriptWrappableVisitor*) const override;

    private:
     explicit V8IdleTask(V8IdleRequestCallback*);

This is pretty clearly wrong.

Interestingly enough, it seems like we usually generate the right insertion... except for one file:
$ grep 'third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59' fixit_output
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:27-59:27}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"

It's unclear if this is specific to the jumbo build or not: in a jumbo build. Here's the current location where the plugin generates the bad replacement:
In file included from gen/third_party/WebKit/Source/core/dom/dom_jumbo_3.cc:6:
In file included from gen/third_party/WebKit/Source/core/dom/../../../../../../../../third_party/WebKit/Source/core/dom/MutationObserver.cpp:44:
In file included from ../../third_party/WebKit/Source/core/probe/CoreProbes.h:36:
In file included from ../../third_party/WebKit/Source/core/dom/Document.h:51:
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:34:32: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
  ~ScriptedIdleTaskController();
                               ^
                                override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{34:32-34:32}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:36:30: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
  void Trace(blink::Visitor*);
                             ^
                              override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{36:30-36:30}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:37:58: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
  void TraceWrappers(const ScriptWrappableVisitor*) const;
                                                         ^
                                                          override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{37:58-37:58}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:47:77: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
    virtual void TraceWrappers(const ScriptWrappableVisitor* visitor) const {}
                                                                            ^
                                                                             override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{47:77-47:77}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:59:27: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
    ~V8IdleTask() = default;
                          ^
                           override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:27-59:27}:" override"
 

Comment 1 by dcheng@chromium.org, Oct 23 2017

Status: Started (was: Assigned)
OK, this appears to be doing the wrong thing when compiling ScriptedIdleTaskController.cpp itself. Not sure what's going on exactly (or why the jumbo build seemed to point elsewhere), but it's likely in the same jumbo unit: perhaps the fact that the definitions are in the same translation unit is causing the error message to point to a different source file as the culprit.

That being said, this does the wrong thing outside jumbo build as well and needs to be fixed.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d514493d6354b2d929e63ade437931cf82d6329

commit 1d514493d6354b2d929e63ade437931cf82d6329
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Oct 24 17:49:53 2017

clang plugin: improve handling of virtual / override fix it hints

Removing virtual is tricky because the AST does not provide the location
of the virtual keyword. Similarly, the AST also does not expose the
location of '=' in |void F() = 0| or |void ~T() = default|.

The plugin previously tries to hack around this in two different ways:
1. for virtual, it assumes the token at |method->getLocStart()| is the
   token that needs to be removed.
2. for override, it scans tokens backwards to try to find a ')' or '=',
   and guesses the appropriate insertion point.

These heuristics break down sometimes, such as when a destructor is
defaulted. This patch cleans up the token scanning logic so it works for
the aforementioned case, and uses the same logic for determining removal
of virtual as well.

This also fixes the plugin to:
1. correctly prioritize retaining the 'final' specifier over the
   'override' specifier
2. when generating the 'override' specifier fixit, also generates a
   removal fixit for 'virtual' if needed so cleanups don't reuqire two
   passes

Bug:  777575 
Change-Id: If7f9df09d3475b4e54596610f2eb5fb477e5cc1a
Reviewed-on: https://chromium-review.googlesource.com/735079
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511206}
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/FindBadConstructsConsumer.cpp
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/FindBadConstructsConsumer.h
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/blacklisted_dirs.txt
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/overridden_methods.cpp
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/overridden_methods.h
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/overridden_methods.txt
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/virtual_base_method_also_final.cpp
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/virtual_base_method_also_final.txt
[modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/virtual_specifiers.txt

Comment 3 by dcheng@chromium.org, Oct 24 2017

Status: Fixed (was: Started)

Sign in to add a comment