Merge pull request #5759 from davepagurek/fix/fes-reserved-regex

Make reserved word assignment regex work better with function calls

Author: limzykenneth

src/core/friendly_errors/sketch_reader.js

@@ -116,8 +116,43 @@ if (typeof IS_MINIFIED !== 'undefined') {
 
   //these regex are used to perform variable extraction
   //visit https://regexr.com/ for the detailed view
-  const varName = /(?:(?:let|const|var)\s+)?([\w$]+)/;
-  const varNameWithComma = /(?:(?:let|const|var)\s+)?([\w$,]+)/;
+  const optionalVarKeyword = /(?:(?:let|const|var)\s+)?/;
+
+  // Bracketed expressions start with an opening bracket, some amount of non
+  // bracket characters, then a closing bracket. Note that this won't properly
+  // parse nested brackets: `constrain(millis(), 0, 1000)` will match
+  // `constrain(millis()` only, but will still fail gracefully and not try to
+  // mistakenly read any subsequent code as assignment expressions.
+  const roundBracketedExpr = /(?:\([^)]*\))/;
+  const squareBracketedExpr = /(?:\[[^\]]*\])/;
+  const curlyBracketedExpr = /(?:\{[^}]*\})/;
+  const bracketedExpr = new RegExp(
+    [roundBracketedExpr, squareBracketedExpr, curlyBracketedExpr]
+      .map(regex => regex.source)
+      .join('|')
+  );
+
+  // In an a = b expression, `b` can be any character up to a newline or comma,
+  // unless the comma is inside of a bracketed expression of some kind (to make
+  // sure we parse function calls with multiple arguments properly.)
+  const rightHandSide = new RegExp('(?:' + bracketedExpr.source + '|[^\\n,])+');
+
+  const leftHandSide = /([\w$]+)/;
+  const assignmentOperator = /\s*=\s*/;
+  const singleAssignment = new RegExp(
+    leftHandSide.source + assignmentOperator.source + rightHandSide.source
+  );
+  const listSeparator = /,\s*/;
+  const oneOrMoreAssignments = new RegExp(
+    '(?:' +
+      singleAssignment.source +
+      listSeparator.source +
+      ')*' +
+      singleAssignment.source
+  );
+  const assignmentStatement = new RegExp(
+    '^' + optionalVarKeyword.source + oneOrMoreAssignments.source
+  );
   const letConstName = /(?:(?:let|const)\s+)([\w$]+)/;
 
   /**
@@ -133,27 +168,12 @@ if (typeof IS_MINIFIED !== 'undefined') {
     //extract variable names from the user's code
     let matches = [];
     linesArray.forEach(ele => {
-      if (ele.includes(',')) {
-        matches.push(
-          ...ele.split(',').flatMap(s => {
-            //below RegExps extract a, b, c from let/const a=10, b=20, c;
-            //visit https://regexr.com/ for the detailed view.
-            let match;
-            if (s.includes('=')) {
-              match = s.match(/(\w+)\s*(?==)/i);
-              if (match !== null) return match[1];
-            } else if (!s.match(new RegExp('[[]{}]'))) {
-              let m = s.match(varName);
-              if (m !== null) return s.match(varNameWithComma)[1];
-            } else return [];
-          })
-        );
-      } else {
-        //extract a from let/const a=10;
-        //visit https://regexr.com/ for the detailed view.
-        const match = ele.match(letConstName);
-        if (match !== null) matches.push(match[1]);
-      }
+      // Match 0 is the part of the line of code that the regex looked at.
+      // Matches 1 and onward will be only the variable names on the left hand
+      // side of assignment expressions.
+      const match = ele.match(assignmentStatement);
+      if (!match) return;
+      matches.push(...match.slice(1).filter(group => group !== undefined));
     });
     //check if the obtained variables are a part of p5.js or not
     checkForConstsAndFuncs(matches);

test/unit/core/error_helpers.js

@@ -974,4 +974,97 @@ suite('Tests for p5.js sketch_reader', function() {
       });
     }
   );
+
+  testUnMinified(
+    'detects reassignment of p5.js functions in declaration lists',
+    function() {
+      return new Promise(function(resolve) {
+        prepSketchReaderTest(
+          ['function setup() {', 'let x = 2, text = 2;', '}'],
+          resolve
+        );
+      }).then(function() {
+        assert.strictEqual(log.length, 1);
+        assert.match(log[0], /you have used a p5.js reserved function/);
+      });
+    }
+  );
+
+  testUnMinified(
+    'detects reassignment of p5.js functions in declaration lists after function calls',
+    function() {
+      return new Promise(function(resolve) {
+        prepSketchReaderTest(
+          [
+            'function setup() {',
+            'let x = constrain(frameCount, 0, 1000), text = 2;',
+            '}'
+          ],
+          resolve
+        );
+      }).then(function() {
+        assert.strictEqual(log.length, 1);
+        assert.match(log[0], /you have used a p5.js reserved function/);
+      });
+    }
+  );
+
+  testUnMinified(
+    'ignores p5.js functions used in the right hand side of assignment expressions',
+    function() {
+      return new Promise(function(resolve) {
+        prepSketchReaderTest(
+          // This will still log an error, as `text` isn't being used correctly
+          // here, but the important part is that it doesn't say that we're
+          // trying to reassign a reserved function.
+          ['function draw() {', 'let x = constrain(100, 0, text);', '}'],
+          resolve
+        );
+      }).then(function() {
+        assert.ok(
+          !log.some(line =>
+            line.match(/you have used a p5.js reserved function/)
+          )
+        );
+      });
+    }
+  );
+
+  testUnMinified(
+    'ignores p5.js function names used as function arguments',
+    function() {
+      return new Promise(function(resolve) {
+        prepSketchReaderTest(
+          ['function draw() {', 'let myLog = (text) => print(text);', '}'],
+          resolve
+        );
+      }).then(function() {
+        assert.strictEqual(log.length, 0);
+      });
+    }
+  );
+
+  testUnMinified(
+    'fails gracefully on inputs too complicated to parse',
+    function() {
+      return new Promise(function(resolve) {
+        prepSketchReaderTest(
+          // This technically is redefining text, but it should stop parsing
+          // after the double nested brackets rather than try and possibly
+          // give a false positive error. This particular assignment will get
+          // caught at runtime regardless by
+          // `_createFriendlyGlobalFunctionBinder`.
+          [
+            'function draw() {',
+            'let x = constrain(millis(), 0, text = 100)',
+            '}'
+          ],
+          resolve
+        );
+      }).then(function() {
+        console.log(log);
+        assert.strictEqual(log.length, 0);
+      });
+    }
+  );
 });