The Wayback Machine - https://web.archive.org/web/20200915222246/https://github.com/uber/NEAL/issues/12
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

Documentation updates and related feedback #12

Open
kastiglione opened this issue Feb 6, 2018 · 13 comments
Open

Documentation updates and related feedback #12

kastiglione opened this issue Feb 6, 2018 · 13 comments

Comments

@kastiglione
Copy link

@kastiglione kastiglione commented Feb 6, 2018

I tried NEAL today and in short time I had a "working" rule. I wanted to provide some feedback on the issues I hit

  1. The readme talks about a rules config file, but doesn't mention a file name. I searched the repo and saw a mention of .rules so I tried that, but it didn't work. At first I thought NEAL was crazy fast but it turns out it just wasn't running any rules.
  2. There is no documentation on > and >> which I found info on through the source and commits. There's one example that uses them, and not many tests that use them.
  3. There is no documentation on var and condition which I noticed in a test, and eventually found more info on in #9.
  4. I was also looking for how to do cross tree sibling/cousin matches, and I think I now have something working, but there was a bunch of trial and error.
  5. I could not get match() to work, and I wasn't able to debug my use of it since I didn't know what the (generated) string it was matching against would look like.
  6. In the examples/tests, I saw one or two uses of property.access syntax, but it's not documented so I'm not clear what it's used for.
  7. I'm getting a number of swift parsing errors, but it doesn't tell me which syntax NEAL is failing on and the error message is funky: SyntaxError}: satisfy.
  8. As a beginner, it's not clear how to mix where clauses with properties that are lists of nodes, or conversely, how properties mix with matchers.
  9. When there is a syntax error in a .rules file, only a line and column number are shown. It would be nice to show the line and a ^ label just like how warnings/errors in source files are shown.
  10. Wish: Print the number of warnings/errors at the end of processing. Even better if it listed counts by rule name.
@kastiglione kastiglione changed the title Documentation updates Documentation updates and related feedback Feb 6, 2018
@tadeuzagallo
Copy link
Contributor

@tadeuzagallo tadeuzagallo commented Feb 8, 2018

Hey, thanks so much for the feedback! I started working on updating the docs and I'll try to address all these points. Just wanted to update the issue, as I couldn't push it today yet.

1 - the name of the file is neal.json, I'll push a commit with the fix for the readme soon, but in the meantime, there's more info about it here if you need.
2 & 3 - I'm finishing updating the docs soon.
4 - You got that working with vars + condition? Let me know if you want to share your rule and I can try to help.
5 - I'll see if I can add another "action", for debugging values of variables currently matched.
6 - Will add that to the docs. The way it works is it just access the property of the matched node. If it's an array, it basically does flatMap.
7 - Yeah, I messed that up when I was improving the performance of the swift parser, not a great tradeoff though :/ If you want to share the files that had syntax errors, I'm happy to debug and fix the parser. I'll also see what I can do about the errors, but that might take a bit longer.
8 - I'll try to explain that better in the docs, but the idea is that if it's a list, it'll execute the rule against each item in the list, i.e. list == "string" will be the same as there exists some x in list such that x == "string".
9 - Will update that! I have to fix it so that the code for printing it in failures can be shared with the core of the framework (which prints syntax errors for rules). Right now the build for NEAL is a bit sketchy, which makes it hard.
10 - You can set the NEAL_STATS=1 when running neal and it will print some info, e.g. the number of failures, warnings and parser failures. There's no number per rule unfortunate, but I can look into adding it. There's documentation about the runtime flags which can be set with environment variables here.

Thanks again for the feedback, I'll push a commit soon referencing this diff.

@kastiglione
Copy link
Author

@kastiglione kastiglione commented Feb 12, 2018

I was also looking for how to do cross tree sibling/cousin matches, and I think I now have something working, but there was a bunch of trial and error.

You got that working with vars + condition? Let me know if you want to share your rule and I can try to help.

I thought I had it working, but nope, it wasn't correct. tl;dr I was trying to identify nested closures that weakly captured self where the outer closure did not capture self weakly. I didn't describe it correctly, it's a ancestor/descendent relationship, not sibling/cousin. This is what led to the > operator, because I want to restrict the weak self check to a closure's direct child CaptureList, not those of nested closures.

@kastiglione
Copy link
Author

@kastiglione kastiglione commented Feb 12, 2018

Thanks for all the answers.

@keith
Copy link

@keith keith commented Feb 22, 2018

Here's one case where we see the SyntaxError when handling double optionals:

final class Foo {
    func bar(string: String??) {
        if case let string?? = string {}
    }
}
@keith
Copy link

@keith keith commented Feb 22, 2018

Here's another case:

final class Foo {
    func bar() {
        switch "" {
        case "foo":
            return
        default:
            break
        }
    }
}
@keith
Copy link

@keith keith commented Feb 22, 2018

Here's another case with multi-line strings:

final class Foo {
    func bar() {
        """
        Shouldn't be able to update a pickup location if there is no
        current ride, or the map isn't fully initialized
        """
    }
}
@keith
Copy link

@keith keith commented Feb 22, 2018

Another case with some weird characters for variable names:

final class Foo {
    func bar() {
        let sinφ = sin(0)
    }
}
@keith
Copy link

@keith keith commented Feb 22, 2018

Another case if you add a newline between your function name and the arguments 🙃

final class Foo {
    func bar
        (completion: @escaping () -> Void) {}
}
@keith
Copy link

@keith keith commented Feb 22, 2018

I think that's most of the case we have, happy to provide more info if that would be useful! This was with neal from homebrew with --HEAD at c40036d

@tadeuzagallo
Copy link
Contributor

@tadeuzagallo tadeuzagallo commented Feb 22, 2018

Thanks for all the examples. Some of these issues are known: we have to add support for Swift 4 features (keypaths will also fail to parse) and there's no unicode support (OCaml doesn't like it). The issue with newlines is a bit more unfortunate indeed, but sadly Swift cares about newlines and (to the best of my knowledge) is not well documented where it's valid to have a newline or not, so it was implement based on trial and error (luckily I had a decent-sized codebase to test it). I'll get working on some of these errors, but let me know if you'd like to contribute :)

tadeuzagallo added a commit that referenced this issue Feb 27, 2018
Fix one of the parser errors related to double optionals (reported in #12)
tadeuzagallo added a commit that referenced this issue Feb 27, 2018
As reported in #12, the Swift parser did not handle identifiers
properly. It did not support the complete list of reserved keywords nor
it handled reserved keywords escaped with backticks properly. This fixes
most of the basic use cases, but does not add support for contextual
keywords yet.
tadeuzagallo added a commit that referenced this issue Feb 27, 2018
As reported in #12, the Swift parser did not handle identifiers
properly. It did not support the complete list of reserved keywords nor
it handled reserved keywords escaped with backticks properly. This fixes
most of the basic use cases, but does not add support for contextual
keywords yet.
tadeuzagallo added a commit that referenced this issue Feb 27, 2018
tadeuzagallo added a commit that referenced this issue Feb 27, 2018
Newlines should be allowed between a function's name and its parameter clause.
Also allow newlines after attributes and add tests.
@tadeuzagallo
Copy link
Contributor

@tadeuzagallo tadeuzagallo commented Feb 28, 2018

@keith all the parse errors you reported, except for unicode support, should be fixed on master. I still want to support unicode, but that's a slightly bigger task, so it might take a bit longer until I get to it. I'll try to publish a new version soon, but let me know if you try it out before.

@kastiglione
Copy link
Author

@kastiglione kastiglione commented Feb 28, 2018

@tadeuzagallo Is there syntax to express the "current node"? I'm finding myself wanting something like that for scoped matchers. For example, using . as "current node":

Node {
  . > Left {
    ...
  }

  . > Right {
    ...
  }
}

The following two don't have the same meaning.

One of Left and/or Right could be non-children descendants:

Node {
  Left {
    ...
  }
  Right {
    ...
  }
}

This guarantees individual parent/child relationship, but doesn't prevent the case where the Nodes are different instances.

Node > Left {
  ...
}
Node > Right {
  ...
}
@tadeuzagallo
Copy link
Contributor

@tadeuzagallo tadeuzagallo commented Mar 1, 2018

@kastiglione Yeah, you're right, that's not supported but it does make a lot of sense. It should be easy to implement, I'll see if I can get this done soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.