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

Implement type checking/validation for filters #4364

Merged
merged 46 commits into from
Apr 9, 2023

Conversation

dnsge
Copy link
Contributor

@dnsge dnsge commented Feb 11, 2023

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

This PR adds type-checking to filters. For example, the filter channel.name == "forsen" && author.badges contains "moderator" can be deduced to evaluate to the type Bool. This allows us to tell the user why their filter is not working. Before, attempting to perform invalid operations like String - Int would silently fail while the filter would purport validity.

This PR was primarily motivated by witnessing conversations in Discord where users were confused about why apparently-valid filters weren't working. Additionally, this PR separates the parsing and execution of filters so the filtering language can be easily used in other parts of Chatterino (if another use-case comes up, like advanced highlighting conditions).

This PR also adjusts some (useless) semantics (for example, 5 + "2" no longer == 7 nor == "7"), so the wiki will require some minor updates.

Most of the code change comes from splitting Expressions into multiple files. synthesizeType() is the primary addition in that realm.

Example of detecting type errors

Consider the filter message.content contains ("hello" || "goodbye"). Someone could conceivably believe this filter allows messages that contain either "hello" or "goodbye" (in fact, this example was taken from Discord). Of course, we can't compute the logical or of two strings, so this filter would silently fail.

Now, the filters page reports that there was an issue with the filter, and opening the detailed output gives:
image

Closes #2466 (in my opinion)

@dnsge dnsge changed the title Initial implementation of filter type validation Implement type checking/validation for filters Feb 11, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/controllers/filters/lang/Filter.cpp Show resolved Hide resolved
src/controllers/filters/lang/Filter.hpp Outdated Show resolved Hide resolved
src/controllers/filters/lang/Types.cpp Outdated Show resolved Hide resolved
src/controllers/filters/lang/Types.cpp Outdated Show resolved Hide resolved
src/controllers/filters/lang/expressions/Expression.cpp Outdated Show resolved Hide resolved
@dnsge dnsge marked this pull request as ready for review February 18, 2023 04:26
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/controllers/filters/FilterRecord.cpp Show resolved Hide resolved
src/controllers/filters/FilterRecord.cpp Show resolved Hide resolved
src/controllers/filters/FilterSet.cpp Show resolved Hide resolved
src/controllers/filters/lang/Filter.cpp Show resolved Hide resolved
src/controllers/filters/lang/Filter.cpp Show resolved Hide resolved
src/widgets/settingspages/FiltersPage.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

@dnsge
Copy link
Contributor Author

dnsge commented Apr 7, 2023

I'm not super fond of how the PossibleType class is set up, could we utilize a variant there instead? I think the variant operator== forwards it to the contained class so it should still be easy to make the API easy to use.

I went ahead and reimplemented PossibleType to make use of std::variant internally (which ends up being much cleaner) in 5474c93.

However, I feel like your comment was more suggesting replacing PossibleType altogether, e.g.

using PossibleType = std::variant<Type, IllTyped>;

This of course would be possible, but I thought that having a wrapper class would still be useful to have utility methods like string() and even well(), given the lovely syntax for checking variants (std::holds_alternative<Type>(variant)).

If you'd like it to make use of only a variant instead of a wrapper, I can implement that.

@pajlada
Copy link
Member

pajlada commented Apr 8, 2023

I'm fine with both IllTyped & PossibleType being classes as they will definitely have some member functions that help them be used, my thinking was more along the lines of:

using PossibleType = std::variant<TypeClass, IllTyped>;

class TypeClass {
  public:
    QString string() const;
    Type unwrap() const;

    bool operator==(Type) const;
    bool operator==(TypeClass) const;
    bool operator==(IllTyped) const;

  private:
  Type type_;
};

class IllTyped {
  public:
    QString description() const;

    bool operator==(Type) const;
    bool operator==(TypeClass) const;
    bool operator==(IllTyped) const;
};

Then in places where we've previously checked if !possibleType.well() { print(possibleType.illTypedDescription()); } we'd instead first unwrap it to either a TypeClass or IllTyped

Happy to hear your thoughts on that sort of solution - I can't see a downside to it but it might become apparent when it's actually done & used 👯

@dnsge
Copy link
Contributor Author

dnsge commented Apr 8, 2023

Updated to use std::variant directly 👍

@dnsge dnsge requested a review from pajlada April 8, 2023 18:41
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@dnsge if you can take a look at the changes I've made, we can get this merged in

@dnsge
Copy link
Contributor Author

dnsge commented Apr 9, 2023

@pajlada Everything looks good 👍

@pajlada pajlada merged commit 34db692 into Chatterino:master Apr 9, 2023
@dnsge dnsge deleted the feat/filter-type-checking branch April 10, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filters are not working
2 participants