EvgeniyPeshkov / syntax-highlighter

Syntax Highlighter extension for Visual Studio Code (VSCode). Based on Tree-sitter.
https://marketplace.visualstudio.com/items?itemName=evgeniypeshkov.syntax-highlighter
MIT License
210 stars 43 forks source link

[Rust] enum variants should be coloured as type #6

Closed Geobert closed 5 years ago

Geobert commented 5 years ago

image

As discussed previously, enum's variant in Rust should be coloured as type. Here Ok and Err are variants of Result. But any variant should be seen a type in Rust. I don't know if tree-sitter distinguish them though.

Geobert commented 5 years ago
pub fn store_session(c: &Connection, auth: &str, user_id: &UserId) -> Result<()> {
    if c.hexists(SESSIONS_LIST, auth)? {
        Err(ServerError::new(
            error::INTERNAL_ERROR,
            "Auth already exists",
        ))
    } else {
        let user_session_key = user_sessions_key(user_id);
        transaction(c, &[SESSIONS_LIST, &user_session_key], |pipe| {
            pipe.hset(SESSIONS_LIST, auth, **user_id)
                .ignore()
                .sadd(&user_session_key, auth)
                .query(c)
        })?;

        Ok(())
    }
}
EvgeniyPeshkov commented 5 years ago

Unfortunately, Tree-sitter treats constructors as functions. And, as we discussed earlier, it's reasonable, because constructor is indeed a special kind of function, that has the same name as type. I'm afraid I cannot highlight them as types. In my opinion, it's not a big deal that constructors, being functions, are highlighted as functions. I believe you don't need special color to distinguish them.

Geobert commented 5 years ago

I understand the technical limitation, but this one is quite important for Rustaceans. So I'll let that open until we can found a solution that doesn't break performances, maybe an improvement from tree-sitter-rust?

EDIT: to make sure, I'm only talking about enum's variant, not calls to struct like:

#[derive(Debug, Deref, PartialEq, Eq)]
pub struct AisleId(pub u32);

fn bla() { 
   let a = AisleId(3);
}

Here I'm expecting AisleId from AisleId(3) to be coloured as a function.

EvgeniyPeshkov commented 5 years ago

I'll try to give one more argument. Assume the next simple and meaningless (= code in C++:

image

Looks good to me: simple, clean, calm, enough.

Now consider that someone wants to highlight primitive types (unsigned int) differently. And someone else says that string from std namespace is very idiomatic, so we should also separate it. And also Outer is type and not namespace/scope (here we've already agreed, but just for example). So what do we get then:

image

It's not readable, it's not meaningful, it's annoying, it seems just random. Such highlighting is just Skittles color bomb. I made only one color for types consciously. This provides for clean, calm and consistent highlighting. I think, that especially because some type is very idiomatic, important or frequent, you don't need to emphasis it, on contrast, it's better to dim such terms making them less aggressive. You won't miss them anyway, your eye will always catch them without additional efforts.

Geobert commented 5 years ago

I understand your point here, but I don't know, having enum's variant with the same color as function call is confusing for me atm.

I'll let the Rust community decides if they are annoyed by that or not ^^

EDIT: again, if tree-sitter doesn't provide the info, it's not fixable without performance impact so just let this floating around, in the hope that tree-sitter-rust project will distinguish them one day :)

EvgeniyPeshkov commented 5 years ago

Just in case, I'll mention ones again, that they are colored like functions, because constructors are special functions and therefore have exactly the same syntax. I don't believe Tree-sitter will be able to distinguish them. It's impossible for syntax parser alone. The same happens in C++ and I was also confused in the first place. Since C++11 there are two ways to call constructor: Class(x) and Class{x}. And former cannot be distinguished from function, while latter is clearly constructor and Class is definitely type.

Geobert commented 5 years ago

Yes I agree with the constructor. But again, enum's variants in Rust are quite a big deal ^^ Maybe nobody else will complain though ^^

Geobert commented 5 years ago

After some though, and as we can't easily do it, I think it's better to close the issue. Anyway, one day, we will have rust-analyzer (next gen Rust compiler front end aiming at IDE integration) that will take care of the highlighting with way more precision than tree-sitter, obviously, being a Rust specific compiler. So it doesn't worth the effort here :)

EvgeniyPeshkov commented 5 years ago

Sure. Please, check the new version, I've published yesterday. It should fix other issues.