Check for missing or duplicated anchors #420

newrushbolt · 2021-11-08T15:18:01Z

Basic anchor\aliases checks, also fix for #395.

coveralls · 2021-11-19T07:10:45Z

Coverage: 99.199% (+0.01%) from 99.184% when pulling 542e05f on newrushbolt:395-detect-missing-anchors into b9e1fd1 on adrienverge:master.

adrienverge

Hello @newrushbolt, thanks for contributing!

The change is great and I'm favorable to merging it, but before I have a few remarks and concerns.

According to the YAML specification:
- It is an error for an alias node to use an anchor that does not previously occur in the document -- https://yaml.org/spec/1.2/spec.html#id2785586
  
  Your commit implements that 👍
  But it waits for the end of the document to report it, whereas it's not needed, and can output errors in a wrong order. Since anchors must be declared before aliases, you should report the problem as soon as the unknown alias is found.
- The alias refers to the most recent preceding node having the same anchor → it's allowed to override anchor -- https://yaml.org/spec/1.2-old/spec.html#id2786448
  
  Your commit implements and documents "Duplicate anchors are not allowed", which disagrees with the spec.

To solve the previous point, and also to prepare the future (if we want to allow the rule to be extended later), I propose to make the rule configurable:

rules:
  anchors:
    forbid-unknown-aliases: true  # true by default, because YAML spec fordids it
    forbid-duplicated-anchors: false  # false by default, because YAML spec allows it

The Python file would look like:

CONF = {'forbid-unknown-aliases': bool,
        'forbid-duplicated-anchors': bool}
DEFAULT = {'forbid-unknown-aliases': True,
           'forbid-duplicated-anchors': False}

if conf['forbid-unknown-aliases']:

I'm not sure whether this new rule (at least forbid-unknown-aliases) should be enabled in the default set of rules: it might break things for many users. On the other hand, broken anchors would mean broken YAML and shouldn't exist...

An option could be to enable it, release a new yamllint version, and if this causes problems for users, you would quickly create a pull request to disable it by default.

What do you think?

tests/rules/test_anchors.py

yamllint/rules/anchors.py

newrushbolt · 2022-01-07T13:58:34Z

Hello @adrienverge, thanks for the review. Sorry for the delay.
Read all the remarks, and realized that this PR had an infinite number of ways to be improved 🙈.

First of all, I decided to split this into two separate rules.
I did some rework, feel free to continue review.

Absolutely
Sure
As well as unknown-aliases is correlates with YAML spec I think it should be enabled by default, and we should add a couple of words in the release note about it.
And I don't think we should revert this behavior even if the sh#t hits the fan after release.
Talking about anchor-duplicates, it should probably stay disabled.

Of course l'll squash all the commits after final approve.

yamllint/rules/unknown_aliases.py

yamllint/rules/anchors.py

adrienverge

Hello Sergei,

Thanks for the good work!

It looks better but I see a problem now: you split the feature into 2 rules, whereas:

they're both related to anchors,
it feels strange to have a rule anchor-duplicates containing an option
with the same name anchor-duplicates. Same for unknown-aliases: {unknown-aliases: true}.

I insist on the following conf, inside a united anchors rule:

CONF = {'forbid-unknown-aliases': bool,
        'forbid-duplicated-anchors': bool}
DEFAULT = {'forbid-unknown-aliases': True,
           'forbid-duplicated-anchors': False}

if conf['forbid-unknown-aliases'] and AliasToken and token.value not in context['anchors']:
    # anchor "{token.value}" is used before assignment
if conf['forbid-duplicated-anchors'] and AnchorToken and token.value in context['anchors']:
    # duplicated anchor "{token.value}"

which is more in the spirit of yamllint. Moreover it would allow extending the rule with more options later.

You can see rules empty-values and octal-values for comparison.

Other than that, please address all my other remarks above and below, so we can merge this 👍

newrushbolt · 2023-01-12T14:06:37Z

@adrienverge is everything ok now?
If so can you merge this request?

amimas

Thanks so much for this PR. Looking forward seeing this merged and released.

In #395, a third option for this rule was also discussed (forbid-unused-anchors). It's similar to the duplicated anchors option. Can that option be added into this PR. I feel that will make this whole.

Thanks again.

amimas · 2023-01-23T01:29:35Z

yamllint/rules/anchors.py

+        if token.value not in context['anchors']:
+            yield LintProblem(
+                token.start_mark.line + 1, token.start_mark.column + 1,
+                f'anchor "{token.value}" is used before assignment')


Should this message also indicate a potentially reference to a non-existent anchor?

Yamllint parses files sequencially, so it does not know whether the anchor is defined later in the file or is totally missing from the document.

(Maybe I didn't get your point, in this case please give example of what you mean :))

The error message as written right now specifically says something like this:

anchor "foo-bar" is used before assignment

But, as you said already

it does not know whether the anchor is defined later in the file or is totally missing from the document.

That's why I was suggesting that the error message probably could be less specific because it's possible that the anchor hasn't been defined in the entire file. It could be something like:

anchor "foo-bar" is used before assignment/defined or does not exist

Hope that makes sense.

@adrienverge - Trying to learn the code, as I'm new to this. Would like to contribute if I can.

Yamllint parses files sequencially, so it does not know whether the anchor is defined later in the file or is totally missing from the document.

Right now problems are reported from each line as tokens are parsed. Is it possible to save them in a global variable instead of reporting right away? Then if the "end of document token" is reached, report the problems? I didn't see any of the existing rule doing it. Also, can errors from multiple locations be reported? In this example: if an anchor is duplicated 3 times, report the 2nd and 3rd occurrences as problem. Not sure if this is valid:

def check(conf, token, prev, next, nextnext, context): # Skipping codes here to keep this snippet short yield LintProblem(....) yield LintProblem(....)

Hope that makes sense.

Yes it does, thanks, I see what you mean now 🙏
What I like in the sentence anchor "foo-bar" is used before assignment is that it's simpler (it counts) and isn't incompatible with "this anchor will actually never be assigned later in the file".
I see this sentence as a good trade-off between simplicity and understandability (compared to anchor "foo-bar" is used before assignment/defined or does not exist)

Right now problems are reported from each line as tokens are parsed. Is it possible to save them in a global variable instead of reporting right away? Then if the "end of document token" is reached, report the problems? I didn't see any of the existing rule doing it.

That looks like a solution, but yamllint is designed to output errors right away, which allows:
- having them sorted naturally,
- being performant and avoid consuming too much RAM,
- not losing past errors if the script crashes at some point.
Changing this would need a major refactor, and I'm not sure this potentially adaptative error message (... used before assignment / ... never assigned) is worth it.

I understand what you're saying and it makes sense. I was just asking because I wanted to look at the forbid-unused-anchors option. Without checking the entire file, not sure how to verify that an anchor isn't being used. Let me know if you have any suggestions.

Maybe we should discuss it in the feature request issue.

See #395 (comment)

adrienverge

@newrushbolt not everything seems to be OK, it looks like the Python linter reports a dozen errors. Can you check it? You can run it locally on your machine with python -m flake8 ..
Apart from that, I haven't re-reviewed the PR thoroughly yet since last year, but if all my remarks were handled it should be good to go.

EDIT: I see you did not apply my request for token.value in context['anchors'], replacing it with context['anchors'].count(token.value), which creates a bug if there are ≥ 3 anchors. Maybe there are other problems. Please check all my remarks and follow contributing guidelines so it doesn't waste your time and mine too much. Thanks!

In #395, a third option for this rule was also discussed (forbid-unused-anchors). It's similar to the duplicated anchors option. Can that option be added into this PR. I feel that will make this whole.

@amimas this development has been open for more than a year, I feel it would be better to just finish and merde it. Then, you can implement this new forbid-unused-anchors option if you'd like too.

adrienverge · 2023-01-27T08:43:12Z

yamllint/rules/anchors.py

+    clients:
+        jack: &jack_client
+            billing_id: 1234
+        bill: &bill_client
+            billing_id: 5678
+    target_client: *jack_client
+
+   the following code snippet would **FAIL**:
+   ::
+
+    clients:
+        jack: &jack_client
+            billing_id: 1234
+        bill: &jack_client
+            billing_id: 5678
+    target_client: *jack_client


All other examples use 2-space indentation: let's be consistent!

adrienverge · 2023-01-27T08:50:12Z

yamllint/rules/anchors.py

+        if token.value not in context['anchors']:
+            yield LintProblem(
+                token.start_mark.line + 1, token.start_mark.column + 1,
+                f'anchor "{token.value}" is used before assignment')


Yamllint parses files sequencially, so it does not know whether the anchor is defined later in the file or is totally missing from the document.

(Maybe I didn't get your point, in this case please give example of what you mean :))

adrienverge · 2023-01-27T08:55:30Z

tests/rules/test_anchors.py

+DUPLICATED_ANCHOR = '''---
+key1: &keyanchor a
+key2: &keyanchor b
+otherkey: *keyanchor
+'''


Please add a test for anchors duplicated 3 times. Latest version of the PR (commit 542e05f) seems to have this bug!

amimas · 2023-02-09T03:22:23Z

@newrushbolt - are you able to look into the remaining feedbacks?

adrienverge · 2023-02-25T14:57:21Z

@newrushbolt we need feedback from you, because @amimas is implementing a new option for your new rule in #537.

I know that yamllint requires high-quality contributions, so it's OK if you don't want to spend time on it. Someone else (maybe me) can implement this new rule in a new pull request. Please tell us what you prefer.

According to the YAML specification [^1]: - > It is an error for an alias node to use an anchor that does not > previously occur in the document. The `forbid-undeclared-aliases` option checks that aliases do have a matching anchor declared previously in the document. Since this is required by the YAML spec, this option is enabled by default. - > The alias refers to the most recent preceding node having the same > anchor. This means that having a same anchor repeated in a document is allowed. However users could want to avoid this, so the new option `forbid-duplicated-anchors` allows that. It's disabled by default. - > It is not an error to specify an anchor that is not used by any > alias node. This means that it's OK to declare anchors but don't have any alias referencing them. However users could want to avoid this, so a new option (e.g. `forbid-unused-anchors`) could be implemented in the future. See #537. Fixes #395 Closes #420 [^1]: https://yaml.org/spec/1.2.2/#71-alias-nodes

adrienverge · 2023-03-19T11:38:49Z

Due to lack of activity here, and the need of @amimas in #537, I've implemented this new rule in #550.

The code is simpler, should be more performant (use of set() and save anchors only when needed), the implementation more robust (it handles anchors duplicated 2 or 3+ times), and the doc now includes the default values. I've also renamed forbid-unknown-aliases to forbid-undeclared-aliases.

@newrushbolt @amimas could you test it and report whether it works for you?

According to the YAML specification [^1]: - > It is an error for an alias node to use an anchor that does not > previously occur in the document. The `forbid-undeclared-aliases` option checks that aliases do have a matching anchor declared previously in the document. Since this is required by the YAML spec, this option is enabled by default. - > The alias refers to the most recent preceding node having the same > anchor. This means that having a same anchor repeated in a document is allowed. However users could want to avoid this, so the new option `forbid-duplicated-anchors` allows that. It's disabled by default. - > It is not an error to specify an anchor that is not used by any > alias node. This means that it's OK to declare anchors but don't have any alias referencing them. However users could want to avoid this, so a new option (e.g. `forbid-unused-anchors`) could be implemented in the future. See #537. Fixes #395 Closes #420 [^1]: https://yaml.org/spec/1.2.2/#71-alias-nodes

adrienverge reviewed Nov 19, 2021

View reviewed changes

newrushbolt force-pushed the 395-detect-missing-anchors branch from b51b44e to b437665 Compare January 7, 2022 13:42

newrushbolt force-pushed the 395-detect-missing-anchors branch 3 times, most recently from cad43ac to 3ad948c Compare January 7, 2022 15:09

adrienverge reviewed Feb 8, 2022

View reviewed changes

yamllint/rules/unknown_aliases.py Outdated Show resolved Hide resolved

yamllint/rules/anchors.py Outdated Show resolved Hide resolved

adrienverge reviewed Feb 8, 2022

View reviewed changes

newrushbolt force-pushed the 395-detect-missing-anchors branch from a52d0eb to aad8c2b Compare February 27, 2022 13:15

newrushbolt added 3 commits January 12, 2023 16:03

Check for missing or duplicated anchors

e105228

Check for missing or duplicated anchors, rework

6ade0ee

Check for missing or duplicated anchors, rework v2

542e05f

newrushbolt force-pushed the 395-detect-missing-anchors branch from aad8c2b to 542e05f Compare January 12, 2023 14:03

amimas reviewed Jan 23, 2023

View reviewed changes

adrienverge reviewed Jan 27, 2023

View reviewed changes

adrienverge mentioned this pull request Feb 8, 2023

yamllint allows referencing a non-existant anchor #395

Closed

amimas mentioned this pull request Feb 12, 2023

feat: add rule for unused anchors #537

Merged

adrienverge mentioned this pull request Mar 19, 2023

anchors: Add new rule to detect undeclared or duplicated anchors #550

Merged

adrienverge closed this in #550 Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for missing or duplicated anchors #420

Check for missing or duplicated anchors #420

newrushbolt commented Nov 8, 2021

coveralls commented Nov 19, 2021 •

edited

adrienverge left a comment

newrushbolt commented Jan 7, 2022

adrienverge left a comment

newrushbolt commented Jan 12, 2023

amimas left a comment

amimas Jan 23, 2023

adrienverge Jan 27, 2023

amimas Jan 28, 2023

amimas Jan 30, 2023

adrienverge Feb 2, 2023

amimas Feb 3, 2023

adrienverge Feb 8, 2023

adrienverge left a comment •

edited

adrienverge Jan 27, 2023

adrienverge Jan 27, 2023

adrienverge Jan 27, 2023

amimas commented Feb 9, 2023

adrienverge commented Feb 25, 2023

adrienverge commented Mar 19, 2023

Check for missing or duplicated anchors #420

Check for missing or duplicated anchors #420

Conversation

newrushbolt commented Nov 8, 2021

coveralls commented Nov 19, 2021 • edited

adrienverge left a comment

Choose a reason for hiding this comment

newrushbolt commented Jan 7, 2022

adrienverge left a comment

Choose a reason for hiding this comment

newrushbolt commented Jan 12, 2023

amimas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienverge left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amimas commented Feb 9, 2023

adrienverge commented Feb 25, 2023

adrienverge commented Mar 19, 2023

coveralls commented Nov 19, 2021 •

edited

adrienverge left a comment •

edited