Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conflict between MD007/ul-indent with start_indented and MD027/no-multiple-space-blockquotes #1473

Open
MatheusBaldi opened this issue Jan 21, 2025 · 8 comments

Comments

@MatheusBaldi
Copy link

MatheusBaldi commented Jan 21, 2025

What is happening?

Markdownlint raises an error for ul-indent when a list is not indented inside a blockquote and start_indented is set to true; when the indentation is fixed, the rule no-multiple-space-blockquote raises an error indicating that a blockquote should not have multiple spaces.

Why is it happening?

When the start_indented option of ul-indent rule is true, it demands a specific starting indentation for a list, anywhere they appear. When inside a blockquote with the rule no-multiple-space-blockquote enabled, both rules try to be applied, negating each other.

Expected behavior

ul-indent with indent bigger than zero and start_indented as true should work normally inside blockquotes, as it used to in previous versions.

This rule already existed and was used before, so why did it only started raising an error now?

  • until recently, the project where the problem happened was using markdownlint-cli2 version 0.7.1, and had it updated to 0.15.0 (released in Nov 9, 2024);
  • in version 0.12.1 (released in Jan 14, 2024) it wasn't being raised, it started in version 0.13.0 (released in Apr 4, 2024);
  • markdownlint was bumped from 0.33.0 to 0.34.0 in version 0.13.0 of markdownlint-cli2. 
    • In version 0.34.0 of markdownlint, the rule MD027/no-multiple-space-blockquote has been entirely reimplemented (730ae9a#diff-c92d9e42e0a36693f8eb86994daf2355d759d71f8fa429c197432c2242a70caaL4596) to fullly use micromark parser instead of markdownit and some helper functions. This is where the bug has been was introduced. Therefore, the rules ul-indent with start_indented option as true, and no-multiple-space-blockquote have been conflicting in every version ever since, which includes the current markdownlint (0.37.4) and, as a consequence, markdownlint-cli2 (0.17.2)

What have I tried?

After finding the complete rewriting of rule MD027 in the version which the error starts, I reverted the implementation of MD027/no-multiple-space-blockquote locally to its previous version and the conflict stopped happening, I was then able to use MD007/ul-indent normally with start_indented set to true inside blockquotes without any lint errors.

Goal of this issue

Bring back to current implementation the behavior where lists could adhere to MD007/ul-indent with start_indented inside blockquotes without conflicting with MD027/no-multiple-space-blockquote.

Setup:

  • node version: 20.12.2
  • npm version: 10.5.2
  • markdownlint-cli2 version: 0.17.2 (last version at the moment of creation of this issue)

Same results were obtained on node 23.13.1 (current lts) and npm 11.0.0 (current latest)

Steps to reproduce:

I've set up a repository here (https://github.com/MatheusBaldi/markdownlint-md027-issue-mre) for convenience. You can clone it, npm install, and then run npx [email protected] ..

If you want to set up your own project, here are the steps:

  • enter a new directory
  • create a .markdownlint.json and a markdown file (example.md) as in the files below
  • run npx [email protected] .

files:

.markdownlint.json:

{
  "ul-indent": {
    "indent": 2,
    "start_indented": true
  },
  "no-multiple-space-blockquote": true
}

examples.md:

# Test cases for conflict between no-multiple-space-blockquote and ul-indent rules

list example without indentation raises an `ul-indent` error:

* first
* second

list with correct indentation doesn't raises an error:

  * first
  * second

Blockquote without multiple spaces doesn't raise an error:

> one
> two
> three

Blockquote with unindented list raises an `ul-indent` error:

> * first
> * second
> * third

Blockquote where the list correctly applies `ul-indent` config raises a
`no-multiple-space-blockquote error`. Nested items raise no error for some reason:

>   * first
>   * second
>   * third
>     * nested 1
>       * nested 2
>         * nested 3 
@DavidAnson
Copy link
Owner

Thank you for the thorough issue!

Here's a simplified demonstration of the behavior you describe: https://dlaa.me/markdownlint/#%25m%23%20Issue%201473%0A%0A%3E%20%20%20%2B%20Item%0A%3E%20%20%20%2B%20Item%0A%3E%20%20%20%20%20%2B%20Item%0A%0A%3C!--%20markdownlint-configure-file%20%7B%0A%20%20%22ul-indent%22%3A%20%7B%0A%20%20%20%20%22indent%22%3A%202%2C%0A%20%20%20%20%22start_indented%22%3A%20true%0A%20%20%7D%2C%0A%20%20%20%20%22no-multiple-space-blockquote%22%3A%20true%0A%7D%20--%3E%0A

The current behavior is inconsistent, but the old behavior was not what you want, either. :)

While I believe this worked as you want in older versions, that was luck. In general, rules are independent and do NOT know about each other. Specifically, they are NOT able to access the configuration for other rules. Because your scenario only arises when MD007 is customized, the lack of MD027's ability to perceive that proves it wasn't behaving as you wish intentionally (you can confirm by looking for params.config in the code for MD027).

Based on code inspection, it seems the current behavior of MD027 handling indented lists differently is because it keys off the micromark linePrefix token to identify extra space and that (essentially) becomes a listItemIndent token for the nested list which means the rule doesn't "see" the extra indent.

blockQuote 
  blockQuotePrefix > 
    blockQuoteMarker >
    blockQuotePrefixWhitespace  
  linePrefix   
  listUnordered 
    listItemPrefix + 
      listItemMarker +
      listItemPrefixWhitespace  
    content Item
      paragraph Item
        data Item
    lineEnding \n
    blockQuotePrefix > 
      blockQuoteMarker >
      blockQuotePrefixWhitespace  
    linePrefix   
    listItemPrefix + 
      listItemMarker +
      listItemPrefixWhitespace  
    content Item
      paragraph Item
        data Item
    lineEnding \n
    blockQuotePrefix > 
      blockQuoteMarker >
      blockQuotePrefixWhitespace  
    listItemIndent     
    listUnordered 
      listItemPrefix + 
        listItemMarker +
        listItemPrefixWhitespace  
      content Item
        paragraph Item
          data Item
lineEnding \n
lineEndingBlank \n

I can fix the inconsistency with blockquoted indented lists, but your scenario will still report violations because MD027 won't know to respect the custom configuration you provided for MD007. In cases like this, I typically advise disabling one of the overlapping rules. I've only superficially looked at this so far, and I'm open to clever solutions about how to make your scenario work without disabling a rule.

@DavidAnson DavidAnson added the bug label Jan 22, 2025
@MatheusBaldi
Copy link
Author

MatheusBaldi commented Jan 24, 2025

I see, thank you for the explanation, it makes sense that they're not aware of each other. I believe the old behavior happened to work because of how it used the list_item_open and list_item_close. I haven't really dug how it was doing that, but inspired by it, I've tried an implementation which looks for some of the list tokens that appear while debugging. It basically ignores the multiple spaces in blockquotes when the next item of the current token is a list type. I'm not sure if that would be ideal to the principle of this rule though, but it looks similar to how it behaved before. Also, it's not complete, the tests are breaking for cases where there is a nested list element with only one level of indentation (lines 68 and 185 in lists-in-blockquote.md). I decided to discuss wether the direction of this implementation is valid or not before going any further on how to fix that. Here is how I've implemented it:

function MD027(params, onError) {
    const { tokens } = params.parsers.micromark;
    for (const token of filterByTypesCached([ "linePrefix" ])) {
      const parent = token.parent;
      const codeIndented = parent?.type === "codeIndented";
      const siblings = parent?.children || tokens;
      const currentTokenIndex = siblings.indexOf(token);
      const previousSiblingType = siblings[currentTokenIndex - 1]?.type;
      const nextSiblingType = siblings[currentTokenIndex + 1]?.type;
      const listTypes = [ "listItemPrefix", "listOrdered", "listUnordered" ];
      if (
        !codeIndented &&
        previousSiblingType === "blockQuotePrefix" &&
        !listTypes.includes(nextSiblingType)
      ) {
        const { startColumn, startLine, text } = token;
        const { length } = text;
        const line = params.lines[startLine - 1];
        addErrorContext(
          onError,
          startLine,
          line,
          undefined,
          undefined,
          [ startColumn, length ],
          {
            "editColumn": startColumn,
            "deleteCount": length
          }
        );
      }
    }
  }

Meanwhile, we'll disable the no-multiple-space-blockquote rule as you suggested. Thank you.

@DavidAnson
Copy link
Owner

I need to play around with this myself before I have an opinion about what changes to make. I should have an opportunity to do that this weekend.

@DavidAnson
Copy link
Owner

Just FYI, I've looked at this a bit and I'm finding more ways no-multiple-space-blockquote struggles within lists. There's the nested list scenario we already touched on, but also the following simpler scenario of a multi-line list item which suffers from the same linePrefix->listItemIndent transformation that leads to partial reporting:
https://dlaa.me/markdownlint/#%25m%23%20Issue%201473%0A%0A%3E%20%20%20%2B%20Item%0A%3E%20%20%20%20%20more%0A%0A%3C!--%20markdownlint-configure-file%20%7B%0A%20%20%22ul-indent%22%3A%20%7B%0A%20%20%20%20%22start_indented%22%3A%20true%0A%20%20%7D%0A%7D%20--%3E%0A

I'm hesitant to try to do too much self-parsing here and therefore inclined to think the current implementation that uses linePrefix is striking a decent balance.

@DavidAnson
Copy link
Owner

I'm changing this from "bug" to "enhancement" because:

  1. It's under-reporting, not over-reporting and there are other rules/scenarios where this is also the case. Under-reporting is the good kind as it doesn't object to valid Markdown.
  2. I haven't thought of a better approach for this. Reporting a violation for the second line of the multi-line list example I give above is tricky without this rule also assuming something about how lists should be structured.
    Using linePrefix as done today seems the most reliable way to determine blockquote space.

In a couple of minutes, I'll commit some test cases I put together for future reference.

@MatheusBaldi
Copy link
Author

Would it be reasonable to add a boolean option like ignoreListIndentation or would it deviate from the rule's purpose?

If acceptable, we would be able to just stop checking multiple spaces in blockquotes for all blockquote lines with list elements, and then safely apply indentation rules specific to lists. This could prevent the conflict scenario.

@DavidAnson
Copy link
Owner

I think that would be reasonable, yes. :) I can't, on brief reflection, think of another situation where this would be a problem. For consistency, I think the parameter should be list_items just like used here: https://github.com/DavidAnson/markdownlint/blob/main/doc/md031.md

I expect to get to this in a few days. Thank you!

@MatheusBaldi
Copy link
Author

That's great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants