Add liquid syntax linter rule (#44150)

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

lib/render-with-fallback.js

@@ -5,7 +5,7 @@ import { TitleFromAutotitleError } from '#src/content-render/unified/rewrite-loc
 class EmptyTitleError extends Error {}
 
 const LIQUID_ERROR_NAMES = new Set(['RenderError', 'ParseError', 'TokenizationError'])
-const isLiquidError = (error) =>
+export const isLiquidError = (error) =>
   error instanceof Error && error.name && LIQUID_ERROR_NAMES.has(error.name)
 
 const isAutotitleError = (error) => error instanceof TitleFromAutotitleError

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

@@ -24,6 +24,7 @@ import { liquidQuotedConditionalArg } from './liquid-quoted-conditional-arg.js'
 import { liquidDataReferencesDefined, liquidDataTagFormat } from './liquid-data-tags.js'
 import { frontmatterFormat } from './frontmatter-format.js'
 import { annotateFrontmatter } from './annotate-frontmatter.js'
+import { frontmatterLiquidSyntax, liquidSyntax } from './liquid-syntax.js'
 import { liquidIfTags, liquidIfVersionTags } from './liquid-versioning.js'
 
 const noDefaultAltText = markdownlintGitHub.find((elem) =>
@@ -60,6 +61,8 @@ export const gitHubDocsMarkdownlint = {
     frontmatterVideoTranscripts,
     frontmatterFormat,
     annotateFrontmatter,
+    frontmatterLiquidSyntax,
+    liquidSyntax,
     liquidIfTags,
     liquidIfVersionTags,
   ],

src/content-linter/lib/linting-rules/liquid-syntax.js

@@ -0,0 +1,97 @@
+import { addError } from 'markdownlint-rule-helpers'
+
+import { getFrontmatter } from '../helpers/utils.js'
+import { liquid } from '#src/content-render/index.js'
+import { isLiquidError } from '../../../../lib/render-with-fallback.js'
+
+/*
+  Attempts to parse all liquid in the frontmatter of a file
+  to verify the syntax is correct.
+*/
+export const frontmatterLiquidSyntax = {
+  names: ['LQ115', 'frontmatter-liquid-syntax'],
+  description:
+    'Frontmatter properties that contain Markdown (e.g., translatable strings) must use valid liquid.',
+  tags: ['liquid', 'frontmatter'],
+  function: function LQ115(params, onError) {
+    const fm = getFrontmatter(params.lines)
+    if (!fm) return
+
+    // Currently this list is hardcoded, but in the future we plan to
+    // use a custom key in the frontmatter to determine which keys
+    // contain Liquid.
+    const keysWithLiquid = ['title', 'shortTitle', 'intro', 'product', 'permissions'].filter(
+      (key) => Boolean(fm[key]),
+    )
+
+    for (const key of keysWithLiquid) {
+      const value = fm[key]
+      try {
+        liquid.parse(value)
+      } catch (error) {
+        // If the error source is not a Liquid error but rather a
+        // ReferenceError or bad type we should allow that error to be thrown
+        if (!isLiquidError(error)) throw error
+        const { errorDescription, columnNumber } = getErrorMessageInfo(error.message)
+        const lineNumber = params.lines.findIndex((line) => line.trim().startsWith(`${key}:`)) + 1
+        // Add the key length plus 3 to the column number to account colon and
+        // for the  space after the key and column number starting at 1.
+        // If there is no space after the colon, a YAMLException will be thrown.
+        const range = [columnNumber + key.length + 3, value.length]
+        addError(
+          onError,
+          lineNumber,
+          'Liquid syntax error: ' + errorDescription,
+          value,
+          range,
+          null, // No fix possible
+        )
+      }
+    }
+  },
+}
+
+/*
+  Attempts to parse all liquid in the Markdown content of a file
+  to verify the syntax is correct.
+*/
+export const liquidSyntax = {
+  names: ['GHD90', 'liquid-syntax'],
+  description: 'Markdown content must have valid liquid.',
+  tags: ['liquid'],
+  function: function GHD90(params, onError) {
+    try {
+      liquid.parse(params.lines.join('\n'))
+    } catch (error) {
+      // If the error source is not a Liquid error but rather a
+      // ReferenceError or bad type we should allow that error to be thrown
+      if (!isLiquidError(error)) throw error
+      const { errorDescription, lineNumber, columnNumber } = getErrorMessageInfo(error.message)
+      const line = params.lines[lineNumber - 1]
+      // We don't have enough information to know the length of the full
+      // liquid tag without doing some regex testing and making assumptions
+      // about if the end tag is correctly formed, so we just give a
+      // range from the start of the tag to the end of the line.
+      const range = [columnNumber, line.slice(columnNumber - 1).length]
+      addError(
+        onError,
+        lineNumber,
+        'Liquid syntax error: ' + errorDescription,
+        line,
+        range,
+        null, // No fix possible
+      )
+    }
+  },
+}
+
+function getErrorMessageInfo(message) {
+  const [errorDescription, lineString, columnString] = message.split(',')
+  // There has to be a line number so we'll default to line 1 if the message
+  // doesn't contain a line number.
+  if (!columnString || !lineString)
+    throw new Error('Liquid error message does not contain line or column number')
+  const lineNumber = parseInt(lineString.trim().replace('line:', ''), 10)
+  const columnNumber = parseInt(columnString.trim().replace('col:', ''), 10)
+  return { errorDescription, lineNumber, columnNumber }
+}

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

@@ -86,6 +86,10 @@ const githubDocsConfig = {
     severity: 'error',
     'partial-markdown-files': false,
   },
+  'liquid-syntax': {
+    severity: 'error',
+    'partial-markdown-files': true,
+  },
   'liquid-if-tags': {
     // LQ114
     severity: 'error',
@@ -118,6 +122,11 @@ export const githubDocsFrontmatterConfig = {
     severity: 'error',
     'partial-markdown-files': false,
   },
+  'frontmatter-liquid-syntax': {
+    // LQ115
+    severity: 'error',
+    'partial-markdown-files': false,
+  },
 }
 
 // Configures rules from the `github/markdownlint-github` repo

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

@@ -4,15 +4,11 @@ import slash from 'slash'
 import walk from 'walk-sync'
 import { zip } from 'lodash-es'
 import yaml from 'js-yaml'
-import { fromMarkdown } from 'mdast-util-from-markdown'
-import { visit } from 'unist-util-visit'
 import fs from 'fs/promises'
 import { existsSync } from 'fs'
 import { jest } from '@jest/globals'
 
-import { frontmatter } from '../../../lib/frontmatter.js'
 import languages from '#src/languages/lib/languages.js'
-import { liquid } from '#src/content-render/index.js'
 import { getDiffFiles } from '../lib/diff-files.js'
 
 jest.useFakeTimers({ legacyFakeTimers: true })
@@ -313,47 +309,6 @@ if (mdToLint.length + ymlToLint.length < 1) {
   })
 }
 
-describe('lint markdown content', () => {
-  if (mdToLint.length < 1) return
-
-  describe.each(mdToLint)('%s', (markdownRelPath, markdownAbsPath) => {
-    let content, ast, links, frontmatterData
-
-    beforeAll(async () => {
-      const fileContents = await fs.readFile(markdownAbsPath, 'utf8')
-      const { data, content: bodyContent } = frontmatter(fileContents)
-
-      content = bodyContent
-      frontmatterData = data
-      ast = fromMarkdown(content)
-
-      links = []
-      visit(ast, ['link', 'definition'], (node) => {
-        links.push(node.url)
-      })
-    })
-
-    test('contains valid Liquid', async () => {
-      // If Liquid can't parse the file, it'll throw an error.
-      // For example, the following is invalid and will fail this test:
-      // {% if currentVersion ! "github-ae@latest" %}
-      expect(() => liquid.parse(content)).not.toThrow()
-    })
-
-    if (!markdownRelPath.includes('data/reusables')) {
-      test('frontmatter contains valid liquid', async () => {
-        const fmKeysWithLiquid = ['title', 'shortTitle', 'intro', 'product', 'permission'].filter(
-          (key) => Boolean(frontmatterData[key]),
-        )
-
-        for (const key of fmKeysWithLiquid) {
-          expect(() => liquid.parse(frontmatterData[key])).not.toThrow()
-        }
-      })
-    }
-  })
-})
-
 describe('lint yaml content', () => {
   if (ymlToLint.length < 1) return
   describe.each(ymlToLint)('%s', (yamlRelPath, yamlAbsPath) => {

src/content-linter/tests/unit/liquid-syntax.js

@@ -0,0 +1,84 @@
+import { runRule } from '../../lib/init-test.js'
+import { frontmatterLiquidSyntax, liquidSyntax } from '../../lib/linting-rules/liquid-syntax.js'
+
+// Configure the test figure to not split frontmatter and content
+const fmOptions = { markdownlintOptions: { frontMatter: null } }
+
+describe(frontmatterLiquidSyntax.names.join(' - '), () => {
+  test('Frontmatter that contains invalid syntax fails', async () => {
+    const markdown = [
+      '---',
+      'title: "{% ata variables.product.product_name %}"',
+      'shortTitle: "{% "',
+      'intro: "{% data reusables.foo.bar }"',
+      'permissions: "{% if true %}Permission statement"',
+      'showMiniToc: "{% if true %}Permission statement"',
+      '---',
+    ].join('\n')
+    const result = await runRule(frontmatterLiquidSyntax, { strings: { markdown }, ...fmOptions })
+    const errors = result.markdown
+    expect(errors.length).toBe(4)
+    expect(errors.map((error) => error.lineNumber)).toEqual([2, 3, 4, 5])
+    expect(errors[0].errorRange).toEqual([9, 40])
+  })
+  test('Frontmatter that contains valid Liquid passes', async () => {
+    const markdown = [
+      '---',
+      "title: '{% data variables.product.product_name %}'",
+      'shortTitle:',
+      "intro: '{% data reusables.foo.bar %}'",
+      "permissions: '{% if true %}Permission statement{% endif %}'",
+      'showMiniToc: true',
+      '---',
+    ].join('\n')
+    const result = await runRule(liquidSyntax, { strings: { markdown }, ...fmOptions })
+    const errors = result.markdown
+    expect(errors.length).toBe(0)
+  })
+})
+
+describe(liquidSyntax.names.join(' - '), () => {
+  test('Missing closing tag syntax in Markdown content fails', async () => {
+    const markdown = ['---', 'title: Title', '---', '{% data reusables.foo.bar }'].join('\n')
+    const result = await runRule(liquidSyntax, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(1)
+    expect(errors[0].lineNumber).toBe(4)
+    expect(errors[0].errorRange).toEqual([1, 27])
+  })
+  test('Misspelled data tag in Markdown content fails', async () => {
+    const markdown = [
+      '---',
+      'title: Title',
+      '---',
+      '{% ata variables.product.product_name %}',
+    ].join('\n')
+    const result = await runRule(liquidSyntax, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(1)
+    expect(errors[0].lineNumber).toBe(4)
+    expect(errors[0].errorRange).toEqual([1, 40])
+  })
+  test('Missing endif tag in Markdown content fails', async () => {
+    const markdown = ['---', 'title: Title', '---', '{% if true %}Permission statement'].join('\n')
+    const result = await runRule(liquidSyntax, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(1)
+    expect(errors[0].lineNumber).toBe(4)
+    expect(errors[0].errorRange).toEqual([1, 33])
+  })
+  test('Valid Liquid syntax in Markdown content passes', async () => {
+    const markdown = [
+      '---',
+      'title: "Title"',
+      '---',
+      '{% data reusables.foo.bar %}',
+      '{% if true %}Permission statement{% endif %}',
+      // Not correct, but not caught by this rule. See liquid-ifversion-tags.
+      '{% ifversion ghhes %}bla{%endif%}',
+    ].join('\n')
+    const result = await runRule(liquidSyntax, { strings: { markdown } })
+    const errors = result.markdown
+    expect(errors.length).toBe(0)
+  })
+})