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

Bug: invalid regex in extend-ignore-identifiers-re silently stops config file parsing #881

Closed
phip1611 opened this issue Dec 7, 2023 · 1 comment · Fixed by #882
Closed
Labels
bug Not as expected

Comments

@phip1611
Copy link
Contributor

phip1611 commented Dec 7, 2023

I found an error in the typos utility, I think. It seems that as soon as one entry in extend-ignore-identifiers-re is invalid

[default]
extend-ignore-identifiers-re = [
  # This entry causes harm; it should be "foo\\(" but no error is reported
  "foo(",
]

the parsing silently fails and causes that all other configurations, such as [default.extend-identifiers] and [default.extend-words] are silently ignored. That's very unfortunate.

One can verify this following these steps:

  • create a .typos.toml with the following content:
    [default]
    extend-ignore-identifiers-re = [
      # This entry causes harm; it should be "foo\\(" but no error is reported
      "foo("
    ]
    
    [default.extend-identifiers]
    bar = "foo"
    
    [default.extend-words]
    bar = "foo"
  • run typos --config .typos.toml --dump-config - | grep 'foo'
  • error you see no results
  • remove the foo( in the config
  • run typos --config .typos.toml --dump-config - | grep 'foo'
  • it works

Expected Behaviour:

Typos should tell which regex is invalid and why it is invalid, instead of silently dropping that error. Furthermore, it should continue parsing the config file.

@phip1611 phip1611 changed the title Bug: invalid regex in extend-ignore-identifiers-re silently drops config file parsing Bug: invalid regex in extend-ignore-identifiers-re silently stops config file parsing Dec 7, 2023
@epage
Copy link
Collaborator

epage commented Dec 8, 2023

My guess as to what is happening is that serde_derive when it sees any error in a flatten of an Option, it uses None rather than being able to distinguish what are recoverable or critical errors.

@epage epage added the bug Not as expected label Dec 8, 2023
epage added a commit to epage/typos that referenced this issue Dec 8, 2023
When `flatten`ing an `Option`, all errors are silenced, making the value
`None`, not just "this field doesn't exist".
The easiest way around this was to not use `Option`.

Not too confident in all of the changes but tests pass?

Fixes crate-ci#881
epage added a commit to epage/typos that referenced this issue Dec 8, 2023
When `flatten`ing an `Option`, all errors are silenced, making the value
`None`, not just "this field doesn't exist".
The easiest way around this was to not use `Option`.

Not too confident in all of the changes but tests pass?

Fixes crate-ci#881
@epage epage closed this as completed in #882 Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Not as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants