From 423395ffb07fd586cc3d78af3d2794896657621f Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Thu, 19 Apr 2018 18:54:59 -0400 Subject: [PATCH 1/5] Report unused private fields They don't get stripped in v0.js --- .eslintrc | 1 + .../eslint-rules/unused-private-field.js | 58 +++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 build-system/eslint-rules/unused-private-field.js diff --git a/.eslintrc b/.eslintrc index d982fd926898..672d1968385b 100644 --- a/.eslintrc +++ b/.eslintrc @@ -46,6 +46,7 @@ "amphtml-internal/no-spread": 2, "amphtml-internal/no-style-property-setting": 2, "amphtml-internal/query-selector": 2, + "amphtml-internal/unused-private-field": 1, "amphtml-internal/todo-format": 0, "amphtml-internal/vsync": 0, "array-bracket-spacing": [2, "never"], diff --git a/build-system/eslint-rules/unused-private-field.js b/build-system/eslint-rules/unused-private-field.js new file mode 100644 index 000000000000..03a0c32564ac --- /dev/null +++ b/build-system/eslint-rules/unused-private-field.js @@ -0,0 +1,58 @@ +/** + * Copyright 2018 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +'use strict'; + +module.exports = function(context) { + const regex = /\.(\w+_) =/g; + return { + MethodDefinition(node) { + if (/test-/.test(context.getFilename())) { + return; + } + + if (node.kind !== 'constructor') { + return; + } + + // Yah, I know, we're not using the AST anymore. + // But there's no good way to do inner traversals, so this is all we got. + const source = context.getSourceCode(); + const constructor = source.getText(node); + const body = source.getText(node.parent).replace(constructor, ''); + + let match; + while ((match = regex.exec(constructor))) { + const name = match[1]; + const member = context.getNodeByRangeIndex(node.range[0] + match.index); + + if (member) { + const comments = context.getCommentsBefore(member); + const testing = comments.some(comment => { + return comment.value.includes('@visibleForTesting'); + }); + if (testing) { + continue; + } + } + + if (!body.includes(name)) { + context.report(member || node, `Unused private variable "${name}".` + + ' If this is used for testing, annotate with "@visibleForTesting".'); + } + } + } + }; +}; From b2e6895de0a845cc41260f398ea9c9cf0bd95702 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 20 Apr 2018 00:05:25 -0400 Subject: [PATCH 2/5] ABCDEFG... --- .eslintrc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslintrc b/.eslintrc index 672d1968385b..7360977e6695 100644 --- a/.eslintrc +++ b/.eslintrc @@ -46,8 +46,8 @@ "amphtml-internal/no-spread": 2, "amphtml-internal/no-style-property-setting": 2, "amphtml-internal/query-selector": 2, - "amphtml-internal/unused-private-field": 1, "amphtml-internal/todo-format": 0, + "amphtml-internal/unused-private-field": 1, "amphtml-internal/vsync": 0, "array-bracket-spacing": [2, "never"], "arrow-parens": [2, "as-needed"], From cb2da6c80b36d69e0cc3b661dd035959e510a216 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 20 Apr 2018 15:52:15 -0400 Subject: [PATCH 3/5] Use AST to find the members --- .../eslint-rules/unused-private-field.js | 58 ++++++++++--------- src/animation.js | 4 +- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/build-system/eslint-rules/unused-private-field.js b/build-system/eslint-rules/unused-private-field.js index 03a0c32564ac..97fb84d26844 100644 --- a/build-system/eslint-rules/unused-private-field.js +++ b/build-system/eslint-rules/unused-private-field.js @@ -16,43 +16,49 @@ 'use strict'; module.exports = function(context) { - const regex = /\.(\w+_) =/g; return { - MethodDefinition(node) { + MemberExpression(node) { if (/test-/.test(context.getFilename())) { return; } - if (node.kind !== 'constructor') { + const { property } = node; + if (property.type !== 'Identifier') { return; } + const { name } = property; + if (!name.endsWith('_')) { + return; + } + + const comments = context.getCommentsBefore(node); + const testing = comments.some(comment => { + return comment.value.includes('@visibleForTesting'); + }); + if (testing) { + return; + } + + const ancestors = context.getAncestors(node); + const constructor = ancestors.reverse().find(node => { + return node.type === 'MethodDefinition' && node.kind === 'constructor'; + }); + + if (!constructor) { + return + } + // Yah, I know, we're not using the AST anymore. // But there's no good way to do inner traversals, so this is all we got. const source = context.getSourceCode(); - const constructor = source.getText(node); - const body = source.getText(node.parent).replace(constructor, ''); - - let match; - while ((match = regex.exec(constructor))) { - const name = match[1]; - const member = context.getNodeByRangeIndex(node.range[0] + match.index); - - if (member) { - const comments = context.getCommentsBefore(member); - const testing = comments.some(comment => { - return comment.value.includes('@visibleForTesting'); - }); - if (testing) { - continue; - } - } - - if (!body.includes(name)) { - context.report(member || node, `Unused private variable "${name}".` + - ' If this is used for testing, annotate with "@visibleForTesting".'); - } + const body = source.getText(constructor.parent).replace( + source.getText(constructor), ''); + + if (!body.includes(name)) { + context.report(node, `Unused private variable "${name}".` + + ' If this is used for testing, annotate with "@visibleForTesting".'); } - } + }, }; }; diff --git a/src/animation.js b/src/animation.js index 5a271e8540c5..6e0f767e0ca4 100644 --- a/src/animation.js +++ b/src/animation.js @@ -170,10 +170,10 @@ class AnimationPlayer { this.startTime_ = 0; /** @private {./time.normtimeDef} */ - this.normLinearTime_ = 0; + // this.normLinearTime_ = 0; /** @private {./time.normtimeDef} */ - this.normTime_ = 0; + // this.normTime_ = 0; /** @private {boolean} */ this.running_ = false; From 33e6bfd6bc6a5aa8ff103e4ad4495f64900b29c5 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 20 Apr 2018 16:12:24 -0400 Subject: [PATCH 4/5] Find unused methods --- .../eslint-rules/unused-private-field.js | 88 +++++++++++++------ 1 file changed, 61 insertions(+), 27 deletions(-) diff --git a/build-system/eslint-rules/unused-private-field.js b/build-system/eslint-rules/unused-private-field.js index 97fb84d26844..840dc48d843b 100644 --- a/build-system/eslint-rules/unused-private-field.js +++ b/build-system/eslint-rules/unused-private-field.js @@ -16,9 +16,59 @@ 'use strict'; module.exports = function(context) { + function stripComments(text) { + // Multi-line comments + text = text.replace(/\/\*(?!.*\*\/)(.|\n)*?\*\//g, function(match) { + // Preserve the newlines + const newlines = []; + for (let i = 0; i < match.length; i++) { + if (match[i] === '\n') { + newlines.push('\n'); + } + } + return newlines.join(''); + }); + // Single line comments either on its own line or following a space, + // semi-colon, or closing brace + return text.replace(/( |}|;|^) *\/\/.*/g, '$1'); + } + + function checkClassUse(node, name) { + if (!name.endsWith('_')) { + return; + } + + const comments = context.getCommentsBefore(node); + const testing = comments.some(comment => { + return comment.value.includes('@visibleForTesting'); + }); + if (testing) { + return; + } + + const ancestors = context.getAncestors(node); + const body = ancestors.reverse().find(node => node.type === 'ClassBody'); + + if (!body) { + return + } + + // Yah, I know, we're not using the AST anymore. + // But there's no good way to do inner traversals, so this is all we got. + const source = context.getSourceCode(); + const bodyText = stripComments(source.getText(body)); + + // Requires two uses of the name to qualify; + const index = bodyText.indexOf(name); + if (!bodyText.includes(name, index + 1)) { + context.report(node, `Unused private "${name}".` + + ' If this is used for testing, annotate with "@visibleForTesting".'); + } + } + return { MemberExpression(node) { - if (/test-/.test(context.getFilename())) { + if (/\btest\b/.test(context.getFilename())) { return; } @@ -28,37 +78,21 @@ module.exports = function(context) { } const { name } = property; - if (!name.endsWith('_')) { - return; - } + checkClassUse(node, name); + }, - const comments = context.getCommentsBefore(node); - const testing = comments.some(comment => { - return comment.value.includes('@visibleForTesting'); - }); - if (testing) { + MethodDefinition(node) { + if (/\btest\b/.test(context.getFilename())) { return; } - const ancestors = context.getAncestors(node); - const constructor = ancestors.reverse().find(node => { - return node.type === 'MethodDefinition' && node.kind === 'constructor'; - }); - - if (!constructor) { - return + const { computed, key } = node; + if (computed) { + return; } - // Yah, I know, we're not using the AST anymore. - // But there's no good way to do inner traversals, so this is all we got. - const source = context.getSourceCode(); - const body = source.getText(constructor.parent).replace( - source.getText(constructor), ''); - - if (!body.includes(name)) { - context.report(node, `Unused private variable "${name}".` + - ' If this is used for testing, annotate with "@visibleForTesting".'); - } - }, + const { name } = key; + checkClassUse(node, name); + } }; }; From 5f7faf5966b410de2eda2ea8a5a3cd0e2518208f Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 20 Apr 2018 16:24:06 -0400 Subject: [PATCH 5/5] Lint --- .../eslint-rules/unused-private-field.js | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/build-system/eslint-rules/unused-private-field.js b/build-system/eslint-rules/unused-private-field.js index 840dc48d843b..5215a33bbb5e 100644 --- a/build-system/eslint-rules/unused-private-field.js +++ b/build-system/eslint-rules/unused-private-field.js @@ -40,7 +40,7 @@ module.exports = function(context) { const comments = context.getCommentsBefore(node); const testing = comments.some(comment => { - return comment.value.includes('@visibleForTesting'); + return /@(visibleForTesting|protected|override)\b/.test(comment.value); }); if (testing) { return; @@ -50,7 +50,7 @@ module.exports = function(context) { const body = ancestors.reverse().find(node => node.type === 'ClassBody'); if (!body) { - return + return; } // Yah, I know, we're not using the AST anymore. @@ -61,8 +61,11 @@ module.exports = function(context) { // Requires two uses of the name to qualify; const index = bodyText.indexOf(name); if (!bodyText.includes(name, index + 1)) { - context.report(node, `Unused private "${name}".` + - ' If this is used for testing, annotate with "@visibleForTesting".'); + context.report(node, `Unused private "${name}".\n` + + '\tIf this is used for testing, annotate with "@visibleForTesting"\n' + + '\tIf this is a protected definition in a base class,' + + ' annotate with "@protected"\n' + + '\tIf this is an override of a protected, annotate with "@override"\n'); } } @@ -72,12 +75,12 @@ module.exports = function(context) { return; } - const { property } = node; + const {property} = node; if (property.type !== 'Identifier') { return; } - const { name } = property; + const {name} = property; checkClassUse(node, name); }, @@ -86,13 +89,13 @@ module.exports = function(context) { return; } - const { computed, key } = node; + const {computed, key} = node; if (computed) { return; } - const { name } = key; + const {name} = key; checkClassUse(node, name); - } + }, }; };