Merge pull request #726 from protofire/fix-natspec-enforce-on-internals

Fix natspec enforce on internals

Author: dbale-altoros

docs/rules/best-practices/use-natspec.md

@@ -57,6 +57,8 @@ This rule accepts an array of options:
 ```
 
 ### Notes
+- For `internal` or `private` function this rule checks natspec as configured, only if there are tags present. If not, function is skipped.
+- For `external` or `public` function this rule checks natspec as specified in the config.
 - If a function or return value has unnamed parameters (e.g. `function foo(uint256)`), the rule only checks the number of `@param` or `@return` tags, not their names.
 - If a function or variable has `@inheritdoc`, the rule skips the validation.
 - The rule supports both `///` and `/** */` style NatSpec comments.

lib/rules/best-practices/use-natspec.js

@@ -202,6 +202,12 @@ const meta = {
       ],
     },
     notes: [
+      {
+        note: 'For `internal` or `private` function this rule checks natspec as configured, only if there are tags present. If not, function is skipped.',
+      },
+      {
+        note: 'For `external` or `public` function this rule checks natspec as specified in the config.',
+      },
       {
         note: 'If a function or return value has unnamed parameters (e.g. `function foo(uint256)`), the rule only checks the number of `@param` or `@return` tags, not their names.',
       },
@@ -253,7 +259,7 @@ class UseNatspecChecker extends BaseChecker {
     if (!hasParameters(node)) tags.splice(tags.indexOf('param'), 1)
     if (!hasReturn(node)) tags.splice(tags.indexOf('return'), 1)
 
-    this.checkNode(node, 'function', tags, isPublicLike)
+    this.checkNode(node, 'function', tags, isPublicLike, isPublicLike)
   }
 
   EventDefinition(node) {
@@ -267,17 +273,24 @@ class UseNatspecChecker extends BaseChecker {
     this.checkNode(node, 'variable', NATSPEC_TARGETS.variable, true)
   }
 
-  checkNode(node, type, overrideTags, requireNoticeIfPublic = false) {
+  checkNode(node, type, overrideTags, requireNoticeIfPublic, isPublicLike = false) {
     const name = node.name || (node.id && node.id.name) || '<anonymous>'
     const tagsRequired = overrideTags || NATSPEC_TARGETS[type] || []
     const startOffset = node.range[0]
     const comments = getLeadingNatSpecComments(startOffset, this.tokens)
-    const tags = extractNatSpecTags(comments)
 
-    // console.log('\n>>> CHECKING', type, name)
-    // console.log('COMMENTS:', comments)
-    // console.log('TAGS:', tags)
+    console.log('isPublicLike :>> ', isPublicLike)
+    console.log('comments.length :>> ', comments.length)
+    console.log('type :>> ', type)
+
+    // check the function is not publicLike and has no tags, avoid checking
+    // if it is internal or private and has natspec comments, check them
+    if (!isPublicLike && comments.length === 0 && type === 'function') return
+
+    // get the tags from comments
+    const tags = extractNatSpecTags(comments)
 
+    // check the function is publicLike and has inheritdoc
     const hasInheritdoc = tags.includes('inheritdoc')
     if (requireNoticeIfPublic && hasInheritdoc) return
 
@@ -369,7 +382,7 @@ function getLeadingNatSpecComments(startOffset, tokens) {
       comments.unshift(value)
     }
 
-    // Si encontramos código real (no comentario ni puntuación), cortamos
+    // If there is real code (not comment or punctuation), break
     if (!value || (!value.startsWith('///') && !value.startsWith('/**'))) {
       if (token.type !== 'Punctuator') break
     }

test/rules/best-practices/use-natspec.js

@@ -543,5 +543,94 @@ describe('Linter - use-natspec', () => {
         "Mismatch in @return count for function 'doSomething'. Expected: 1, Found: 0"
       )
     })
+
+    it('should ignore internal and private functions if there are no tags', () => {
+      const code = `
+                    contract HalfIgnored {
+
+                      function doSomething1(uint256 amount) internal{}
+
+                      function doSomething2(uint256 amount) private{}
+
+                      /// @dev documented internal function
+                      function doSomething2(uint256 amount) private{}
+                    }
+                  `
+
+      const report = linter.processStr(code, {
+        rules: {
+          'use-natspec': [
+            'error',
+            {
+              title: {
+                enabled: false,
+              },
+              author: {
+                enabled: false,
+              },
+              notice: {
+                enabled: true,
+                ignore: {
+                  contract: ['HalfIgnored'],
+                },
+              },
+              param: {
+                enabled: true,
+              },
+            },
+          ],
+        },
+      })
+      assertNoErrors(report)
+    })
+
+    it('should check internal and private functions if there are tags', () => {
+      const code = `
+                    contract HalfIgnored {
+
+                      /// @notice does something
+                      /// @param amount1 the amount
+                      function doSomething1(uint256 amount) internal returns(uint256){}
+                    }
+                  `
+
+      const report = linter.processStr(code, {
+        rules: {
+          'use-natspec': [
+            'error',
+            {
+              title: {
+                enabled: false,
+              },
+              author: {
+                enabled: false,
+              },
+              notice: {
+                enabled: true,
+                ignore: {
+                  contract: ['HalfIgnored'],
+                },
+              },
+              param: {
+                enabled: true,
+              },
+              return: {
+                enabled: true,
+              },
+            },
+          ],
+        },
+      })
+      assertErrorCount(report, 3)
+      assert.ok(
+        report.reports[0].message,
+        "Mismatch in @param names for function 'doSomething1'. Expected: [amount], Found: [amount1]"
+      )
+      assert.ok(report.reports[1].message, "Missing @return tag in function 'doSomething1'")
+      assert.ok(
+        report.reports[2].message,
+        "Mismatch in @return count for function 'doSomething'. Expected: 1, Found: 0"
+      )
+    })
   })
 })