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

Ideas for inspections #18

Open
dnbln opened this issue Nov 18, 2020 · 4 comments
Open

Ideas for inspections #18

dnbln opened this issue Nov 18, 2020 · 4 comments
Labels
enhancement New feature or request

Comments

@dnbln
Copy link
Contributor

dnbln commented Nov 18, 2020

Named / unnamed symbols (implemented)

Currently lalrpop denies named and unnamed symbols to be a part of the same alternative. Example:

 file.lalrpop: ... error: anonymous symbols like this one cannot be combined with named symbols like `named:NamedFuncArgs`

  --- stderr
    	<PositionalFuncArgs> SingleComma <named: NamedFuncArgs> SingleComma? => FuncCallArgs::new(<>),
       ~~~~~~~~~~~~~~~~~~~~

All the alternatives have action code and the type of the nonterminal wasn't specified (implemented)

Example:

Num = r"[0-9]+" => i32::from_str(<>).unwrap();

Note that lalrpop is able to infer it in cases like(from the second alternative):

Num: i32 = r"[0-9]+" => i32::from_str(<>).unwrap();

Expr = {
    <e: Expr> "+" <n: Num> => e + n,
    Num,
}

After this is implemented, add a quick fix by interrogating the rust plugin about the type and set it on the nonterminal. (implemented)

Maybe also as an intent? Implemented

Nonterminal reference cannot be resolved (implemented)

Example

// nothing in this lalrpop file aside this:
grammar;

A = {
    B,
}

Also quickfix: depending on the form; if it looks like this:

A = {
    B,
}

Then it could be quick-fixed either by adding it to A, like:

A<B> = {
     B,
}

Or by creating it:

A = {
    B,
}

B = {
}

While if it looks like this:

// C exists somewhere
A = {
    B<C>,
}

It could only be fixed by creating it:

// C exists somewhere
A = {
    B<C>,
}

B<Rule> = {
}

Nonterminal reference doesn't have the expected number of "arguments"

Example:

A = {
    B,
}

B<Rule> = {
    Rule,
}

Quick fix: add / remove until the number is right.

And the hardest, most brutal of them all.

Detect ambiguities in the grammar.

Honestly I have no idea what black magic happens here in the first place.

@dnbln dnbln mentioned this issue Nov 27, 2020
@AzureMarker
Copy link
Owner

Another idea: detect when a nonterminal references itself and uses type inference. LALRPOP uses a stack and returns an error here:
https://github.com/lalrpop/lalrpop/blob/20832c3a6e829ceea98f65b0dd301aaa7fd3aa6c/lalrpop/src/normalize/tyinfer/mod.rs#L166

Our type resolver currently returns () as the type if this is detected.

@dnbln
Copy link
Contributor Author

dnbln commented Nov 29, 2021

Another idea: detect when a nonterminal references itself and uses type inference. LALRPOP uses a stack and returns an error here: https://github.com/lalrpop/lalrpop/blob/20832c3a6e829ceea98f65b0dd301aaa7fd3aa6c/lalrpop/src/normalize/tyinfer/mod.rs#L166

Our type resolver currently returns () as the type if this is detected.

We can try something else: pick other alternatives to try to resolve the type, if available.

Say you have the following:

Recursive = {
    Recursive,
    NotRecursive,
};

NotRecursive: () = {};

It should be possible to infer the type of Recursive, knowing there is the NotRecursive alternative that would not lead to recursion.

Also, this may have a problematic time complexity.

@AzureMarker
Copy link
Owner

We should keep compatibility with LALRPOP though, which currently returns an error if it detects this case (see link in previous comment). Note that it only does that if the type is implicit. The "NotRecursive" example you shared does (I think, haven't tested yet) get rejected by LALRPOP because it sees the recursion while resolving the type of Recursive.

I'll try to test this later to verify that this is indeed the case.

@AzureMarker
Copy link
Owner

Interesting. I tried this out and LALRPOP seems to accept it. I guess we have to figure out how to do this too. Luckily we can look at LALRPOP's code :).

src/parser.lalrpop:

grammar;

pub Recursive = {
    "r" <Recursive>,
    NotRecursive,
};

NotRecursive: &'static str = "n" => "test";

src/main.rs:

use lalrpop_util::lalrpop_mod;
use std::io::{self, Read};

lalrpop_mod!(parser);

fn main() {
    let mut input = String::new();
    io::stdin().read_to_string(&mut input).unwrap();

    let output = parser::RecursiveParser::new().parse(&input);

    println!("Output: {:?}", output);
}

Example:

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/lalrpop-test`
rrn
Output: Ok("test")

@AzureMarker AzureMarker added the enhancement New feature or request label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants