Liquid linter rule conditional quotes (#43013)

Co-authored-by: Peter Bengtsson <peterbe@github.com>

src/content-linter/lib/helpers/liquid-utils.js

@@ -0,0 +1,43 @@
+import { Tokenizer } from 'liquidjs'
+
+const liquidTokenCache = new Map()
+
+export function getLiquidTokens(content) {
+  if (!content) return []
+
+  if (liquidTokenCache.has(content)) {
+    return liquidTokenCache.get(content)
+  }
+
+  const tokenizer = new Tokenizer(content)
+  const tokens = tokenizer.readTopLevelTokens()
+  liquidTokenCache.set(content, tokens)
+  return liquidTokenCache.get(content)
+}
+
+export const OUTPUT_OPEN = '{%'
+export const OUTPUT_CLOSE = '%}'
+export const TAG_OPEN = '{{'
+export const TAG_CLOSE = '}}'
+
+export const conditionalTags = ['if', 'elseif', 'unless', 'case', 'ifversion']
+
+export function getPositionData(token, lines) {
+  // Liquid indexes are 0-based, but we want to
+  // covert to the system used by Markdownlint
+  const begin = token.begin + 1
+  const end = token.end + 1
+  // Account for the newline character at the end
+  // of each line that is not represented in the
+  // `lines` array
+  const lineLengths = lines.map((line) => line.length + 1)
+
+  let count = begin
+  let lineNumber = 1
+  for (const lineLength of lineLengths) {
+    if (count - lineLength <= 0) break
+    count = count - lineLength
+    lineNumber++
+  }
+  return { lineNumber, column: count, length: end - begin }
+}

src/content-linter/lib/linting-rules/index.js

@@ -11,6 +11,7 @@ import { listFirstWordCapitalization } from './list-first-word-capitalization.js
 import { linkPunctuation } from './link-punctuation.js'
 import { earlyAccessReferences } from './early-access-references.js'
 import { yamlScheduledJobs } from './yaml-scheduled-jobs.js'
+import { liquidQuotedConditionalArg } from './liquid-quoted-conditional-arg.js'
 
 export const gitHubDocsMarkdownlint = {
   rules: [
@@ -26,5 +27,6 @@ export const gitHubDocsMarkdownlint = {
     linkPunctuation,
     earlyAccessReferences,
     yamlScheduledJobs,
+    liquidQuotedConditionalArg,
   ],
 }

src/content-linter/lib/linting-rules/liquid-quoted-conditional-arg.js

@@ -0,0 +1,56 @@
+import { TokenKind } from 'liquidjs'
+import { addError } from 'markdownlint-rule-helpers'
+
+import { getLiquidTokens, conditionalTags, getPositionData } from '../helpers/liquid-utils.js'
+import { isStringQuoted } from '../helpers/utils.js'
+
+/*
+  Checks for instances where a Liquid conditional tag's argument is
+   quoted because it will always evaluate to true.
+
+  For example, the following would be flagged:
+  {% if "foo" %}
+  {% ifversion "bar" %}
+*/
+export const liquidQuotedConditionalArg = {
+  names: ['LQ111', 'liquid-quoted-conditional-arg'],
+  description: 'Liquid conditional tags should not quote the conditional argument.',
+  tags: ['liquid', 'formatting'],
+  function: function LQ111(params, onError) {
+    const content = params.lines.join('\n')
+    const tokens = getLiquidTokens(content)
+      .filter((token) => token.kind === TokenKind.Tag)
+      .filter((token) => conditionalTags.includes(token.name))
+      .filter((token) => {
+        const tokensArray = token.args.split(/\s+/g)
+        if (tokensArray.some((arg) => isStringQuoted(arg))) return true
+        return false
+      })
+
+    if (!tokens.length) return
+
+    for (const token of tokens) {
+      const lines = params.lines
+      const { lineNumber, column, length } = getPositionData(token, lines)
+      // LineNumber starts at 1, but lines is 0-based
+      const line = lines[lineNumber - 1].slice(column - 1, column + length)
+      // Trim the first and last character off of the token args
+      const replaceWith = token.args.slice(1, token.args.length - 1)
+      const replaceString = line.replace(token.args, replaceWith)
+
+      addError(
+        onError,
+        lineNumber,
+        "A conditional Liquid tag's argument is quoted, meaning it will always evaluate to true. Remove the quotes to allow Liquid to evaluate variable.",
+        token.content,
+        [column, length],
+        {
+          lineNumber,
+          editColumn: column,
+          deleteCount: length,
+          insertText: replaceString,
+        },
+      )
+    }
+  },
+}

src/content-linter/style/github-docs.js

@@ -53,6 +53,11 @@ export const githubDocsConfig = {
     severity: 'error',
     'partial-markdown-files': true,
   },
+  'liquid-quoted-conditional-arg': {
+    // LQ111
+    severity: 'error',
+    'partial-markdown-files': true,
+  },
 }
 
 export const searchReplaceConfig = {

src/content-linter/tests/lint-versioning.js

@@ -57,10 +57,6 @@ describe('lint feature versions', () => {
 
 const allFiles = walkFiles('content', '.md').concat(walkFiles('data', ['.yml', '.md']))
 
-// Quoted strings in Liquid, like {% if "foo" %}, will always evaluate true _because_ they are strings.
-// Instead we need to use unquoted variables, like {% if foo %}.
-const stringInLiquidRegex = /{% (?:if|ifversion|elseif|unless) (?:"|').+?%}/g
-
 // Make sure the `if` and `ifversion` Liquid tags in content and data files are valid.
 describe('lint Liquid versioning', () => {
   describe.each(allFiles)('%s', (file) => {
@@ -89,14 +85,6 @@ describe('lint Liquid versioning', () => {
     ${ifsForVersioning.join('\n')}`
       expect(ifsForVersioning.length, errorMessage).toBe(0)
     })
-
-    test('does not contain Liquid that evaluates strings (because they are always true)', async () => {
-      const matches = fileContents.match(stringInLiquidRegex) || []
-      const message =
-        'Found Liquid conditionals that evaluate a string instead of a variable. Remove the quotes around the variable!'
-      const errorMessage = `${message}\n  - ${matches.join('\n  - ')}`
-      expect(matches.length, errorMessage).toBe(0)
-    })
   })
 })
 

src/content-linter/tests/unit/liquid-quoted-conditional-args.js

@@ -0,0 +1,104 @@
+import { runRule } from '../../lib/init-test.js'
+import { liquidQuotedConditionalArg } from '../../lib/linting-rules/liquid-quoted-conditional-arg.js'
+
+describe(liquidQuotedConditionalArg.names.join(' - '), () => {
+  test('if conditional with quote args fails', async () => {
+    const markdown = [
+      '---',
+      'title: Good sample page',
+      '---',
+      '',
+      ' - One',
+      '{% if product.title == "Awesome Shoes" %}',
+      "{% elseif 'ghes' %}",
+      '{% elseif "ghec" %}',
+      '{% endif %}',
+      '{% data variables.stuff.foo%}',
+    ].join('\n')
+    const result = await runRule(liquidQuotedConditionalArg, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(2)
+    expect(errors.map((error) => error.lineNumber)).toEqual([7, 8])
+    expect(errors[0].errorRange).toEqual([1, 19])
+    expect(errors[1].errorRange).toEqual([1, 19])
+  })
+  test('ifversion conditional with quote args fails', async () => {
+    const markdown = [
+      '---',
+      'title: Good sample page',
+      '---',
+      '',
+      ' - One',
+      '{% ifversion "ghec" %}',
+      '{% ifversion "fpt" or ghec %}',
+      '{% ifversion fpt and "ghec" %}',
+      '{{name | capitalize}}',
+      '{% endif %}',
+      '{% data variables.stuff.foo%}',
+    ].join('\n')
+    const result = await runRule(liquidQuotedConditionalArg, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(3)
+    expect(errors.map((error) => error.lineNumber)).toEqual([6, 7, 8])
+    expect(errors[0].errorRange).toEqual([1, 22], [1, 29], [1, 23])
+  })
+  test('unless conditional with quote args fails', async () => {
+    const markdown = [
+      '---',
+      'title: Good sample page',
+      '---',
+      '',
+      ' - One',
+      '{% unless "this" %}',
+      '- Three',
+      '{% data variables.stuff.foo%}',
+    ].join('\n')
+    const result = await runRule(liquidQuotedConditionalArg, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(1)
+    expect(errors.map((error) => error.lineNumber)).toEqual([6])
+    expect(errors[0].errorRange).toEqual([1, 19])
+  })
+  test('case conditional with quote args fails', async () => {
+    const markdown = [
+      '---',
+      'title: Good sample page',
+      '---',
+      '',
+      '{% case "product.type" %}',
+      "{% when 'Health' %}",
+      'This is a health potion.',
+      '{% when "Love" %}',
+      'This is a love potion.',
+      '{% else %}',
+      'This is a potion.',
+      '{% endcase %}',
+      '- Three',
+      '{% data variables.stuff.foo%}',
+    ].join('\n')
+    const result = await runRule(liquidQuotedConditionalArg, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(1)
+    expect(errors.map((error) => error.lineNumber)).toEqual([5])
+    expect(errors[0].errorRange).toEqual([1, 25])
+  })
+  test('conditional without quote args pass', async () => {
+    const markdown = [
+      '---',
+      'title: Good sample page',
+      '---',
+      '',
+      '{% case product.type %}',
+      "{% when 'Health' %}",
+      '{% unless this %}',
+      '{% ifversion ghec %}',
+      '{% elseif ghes %}',
+      '{% if ghae %}',
+      '- Three',
+      '{% data variables.stuff.foo%}',
+    ].join('\n')
+    const result = await runRule(liquidQuotedConditionalArg, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(0)
+  })
+})