Skip to content
This repository has been archived by the owner on Oct 16, 2020. It is now read-only.

Nano Linter issues mega thread #1

Open
6 tasks
jspenguin2017 opened this issue Dec 7, 2017 · 20 comments
Open
6 tasks

Nano Linter issues mega thread #1

jspenguin2017 opened this issue Dec 7, 2017 · 20 comments
Labels
Milestone

Comments

@jspenguin2017
Copy link
Member

jspenguin2017 commented Dec 7, 2017

Filter Linter

Show warning if:

  • Too many or too few arguments are passed in for a script snippet
  • Filter is platform specific and current platform does not support it
  • A script snippet injection rule only have negated domain (it will be converted to an exception rule)
  • A filter will match word boundary but the boundary character is not explicitly given
  • A common mistake is detected

Common mistakes:

  • ||example.com^$generichide - Forgot @@ or important
    It is quite rare that someone want to use important on generichide, chances are he forgot @@.
    Detection logic: If a filter has generhichide but not important and is not an exception filter, then dispatch a warning.
  • /example-path/ - Forgot trailing *
  • /example-path/im - Regular expression flags in procedural cosmetic filters gorhill/uBlock#3372
    When matching part of a path, trailing wildcard is required, otherwise it becomes a RegExp rule.
    Detection logic: If a RegExp rule only contains letters, numbers, -, _, and ., then dispatch a warning.
  • |example.com^ - Missed one |
    Can happen when copying filter, didn't select the whole line.
    Detection logic: For start anchor, if what is anchored does not include protocol, dispatch a warning.
  • ||example.com - Forgot ^
    Although the filter works, it is usually a mistake as it can match example.communicate.example.org.
    Detection logic: If a filter has host name anchor but no ^ nor : nor /, dispatch a warning.
  • example.blogspot.*##element - Wildcard matching half of public suffix
    Filter will not work as expected.
    Detection logic: Could be difficult to implement.

Whitelist Linter

Show warning if:

  • RegExp is used but that is unlikely to be the intent of the user
@jspenguin2017 jspenguin2017 modified the milestones: v1.0.2, Future Dec 7, 2017
@jspenguin2017 jspenguin2017 changed the title Add filter linter Nano Linter issues mega thread Dec 13, 2017
@jspenguin2017 jspenguin2017 modified the milestones: Future, v1.0.1 Dec 23, 2017
@jspenguin2017

This comment has been minimized.

@ameshkov

This comment has been minimized.

@gorhill

This comment has been minimized.

@ameshkov

This comment has been minimized.

@gorhill

This comment has been minimized.

@jspenguin2017

This comment has been minimized.

@ameshkov

This comment has been minimized.

@jspenguin2017

This comment has been minimized.

@grenzor
Copy link

grenzor commented Feb 27, 2018

Some Linter issues:

  1. ||example.com^$~document discards the filter and gives a cannot be negated error, but document can be negated and the filter should be valid

  2. @||example.com^ no error is given but filter is missing a @ at the start

  3. ||example.com^,domain=example2.com no error is given but it's an invalid filter

  4. ||example.com^$third-party,$image accidental use of $ for image makes it invalid

  5. x||example.com^ becomes invalid filter due to x, usually due to some typo or accidental copying

  6. example.blogspot.*##element error should be given whenever trying to match partial public suffix. Might also be applicable to other static filter syntax

  7. ||example.com^$third-party,~image,image looks like image block takes precedence over negated. Not sure if some error could be given for this kind of accidental double syntax

@jspenguin2017
Copy link
Member Author

jspenguin2017 commented Feb 27, 2018

@grenzor
The linter is always right when it comes to showing errors since it uses the exact same parsing engine. If it shows error, then the filter is rejected, if it does not show error, then the filter is accepted, although how will the filter behave is another question.

By design, the linter will never show error if the filter is accepted, but I do agree warnings should be shown for some of these cases. I'll work on that later.

I think these should generate warning:

  • @||example.com^
  • example.blogspot.*##element
  • ||example.com^$third-party,~image,image

The error in ||example.com^,domain=example2.com and ||example.com^$third-party,$image are pretty obvious from syntax highlighting, so no extra linter warning is needed. Also, I think x||example.com^ is really unlikely to happen.

@grenzor
Copy link

grenzor commented Feb 28, 2018

The error in ||example.com^,domain=example2.com and ||example.com^$third-party,$image are pretty obvious from syntax highlighting

They may be easy to spot if you have a few filters, but for someone with 100s/1000s of their own filters it is not easy at all. It's easier for the user to scroll down and just look at the sidebar for an icon of error or warning than manually checking each line's color syntaxing.

@jspenguin2017
Copy link
Member Author

jspenguin2017 commented Feb 28, 2018

@grenzor
I think you should be writing one filter at a time, and not 1000 then test together. Beside, even if the filter doesn't contain obvious mistake, it doesn't mean it'll work.

@grenzor
Copy link

grenzor commented Feb 28, 2018

I agree, but my use case was someone with already written filters in uBO who then imported them into Nano.

@jspenguin2017
Copy link
Member Author

jspenguin2017 commented Feb 28, 2018

@grenzor
You should still have validated each filter when you wrote them to make sure they actually work.

@jspenguin2017

This comment has been minimized.

@grenzor

This comment has been minimized.

@jspenguin2017

This comment has been minimized.

@ameshkov

This comment has been minimized.

@jspenguin2017

This comment has been minimized.

@jspenguin2017

This comment has been minimized.

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

No branches or pull requests

4 participants