Allow idents to start with `_` #244

GuillaumeGomez · 2024-11-14T22:11:09Z

Until now, we didn't allow _x as ident. Fixed it. :)

Kijewski · 2024-11-15T08:08:20Z

rinja_parser/src/lib.rs

-        (None, name, []) if name.chars().next().map_or(true, char::is_lowercase) => {
+        (None, name, [])
+            if name
+                .chars()
+                .next()
+                .map_or(true, |c| c == '_' || c.is_lowercase()) =>


I'm kinda confused if it is okay that this function returns PathOrIdentifier::Identifier("_") for the placeholder with this change. Before, PathOrIdentifier::Path(vec!["_"]) would be returned. Actually, both look wrong to me.

Can you have a look what happens of you add a case ↓ before the changed one?

(None, "_", _) => fail.parse_next(initial_i),

it didn't change anything as far as I could see.

Kijewski · 2024-11-15T08:09:17Z

Good catch! Interesting that this case did not occur earlier.

GuillaumeGomez · 2024-11-15T13:56:53Z

The original patch was not enough so I greatly extended it. It is now much more useful. :)

Kijewski · 2024-11-18T13:47:12Z

Sorry, I'm not quite happy with the second commit. IMHO on the left-hand side of an assignment, the target, placeholders should always be allowed without restrictions. Maybe you are are only interested in the side-effect of an assignment {% let _ = it.next() %}, or it must have a specific form {% let [_] = slice.try_into().unwrap() %}.

Maybe you could remove the 2nd commit from this PR and move it into a new one, and I'll have a look at it, then?

GuillaumeGomez · 2024-11-18T14:16:29Z

Good point. Wouldn't you prefer that I update the second commit to allow it instead?

Kijewski · 2024-11-18T14:23:23Z

I have no strong opinion. :) We can keep it in one PR.

GuillaumeGomez · 2024-11-18T14:24:23Z

Then leave me some time so I can check what actual cases are allowed on the lhs.

Kijewski · 2024-11-18T14:26:20Z

Take as much time as you need. :)

cipriancraciun · 2024-11-18T18:17:23Z

I think I've opened this can of worms with my initial issue on Askama: https://github.com/djc/askama/issues/1105

(I've tested all of the following examples on the current pull request commit GuillaumeGomez@05ae1ba)

In essence, for variable names I think the following cases should be covered as they are allowed by Rust (and most other programming languages):

{% let _ = 42 %} -- fails with error: reserved keyword _ cannot be used here; why would someone use this?
- perhaps for the side-effect;
- perhaps when refactoring some code where we one doesn't want to change too much;
{% let _x = 42 %} or {% let _X = 42 %} -- my initial issue, which fails with latest Rinja release, works with this pull request; why would someone use this?
- for example I always prefix variable names with _ to highlight that they are locals, and not some crate or something imported; (I know that in Rust an identifier starting with _ is treated specially by the compiler, but it works;)
- any other reason, like for example coming from Python, and creating some templates for Rinja;
- for example one could use __, i.e. two (or more) underscores, as to mean "reserved for base and partials to communicate between themselves", etc., just as the Python convention;
{% let X = 42 %} (i.e. uppercase variables) -- fails with error: literals are not allowed on the left-hand side of an assignment; why would someone use this?
- for example in a base template, or perhaps in an include, someone would like to define a variable that is all upper-case to denote a constant;

Thus, it would be nice before merging this pull-request, to cover a few more corner-cases.

It would be nice (unless it conflicts with the Jinja syntax) that all Rust valid identifiers (as specified in https://doc.rust-lang.org/reference/identifiers.html) to be considered valid identifiers for let, for, match and other places where identifiers are to be used.

GuillaumeGomez · 2024-11-18T22:05:03Z

{% let X = 42 %}

For this one, please open another issue. For the rest, I'll add a regression test for them.

GuillaumeGomez · 2024-11-18T22:13:18Z

I re-allowed the few cases that should not be forbidden. :)

Kijewski · 2024-11-19T17:01:27Z

rinja_derive/src/generator.rs

@@ -1118,6 +1118,7 @@ impl<'a> Generator<'a> {
                    _ => Ok(false),
                }
            }
+            Target::Placeholder("_") => Ok(false),


Isn't wildcard .. handling needed, too?

Target::Rest(var) => match &**var { Some(var) => "what's done in Target::Name(_)", None => Ok(false), }

Very good point!

GuillaumeGomez · 2024-11-19T17:11:51Z

Added handling for ...

Kijewski · 2024-11-19T19:48:48Z

rinja_derive/src/generator.rs

@@ -997,6 +997,7 @@ impl<'a> Generator<'a> {
                    _ => Ok(false),
                }
            }
+            Target::Placeholder("_" | "..") => Ok(false),


The second case will never be true, though. Target::Placeholder(_) only ever captures "_", and Target::Rest(_) captures .. or .. @ var.

Both variant should probably contain a WithSpan<'a, ()> instead of the captured token, to make the meaning more clear.

Ok, gonna clean up this code because there is no point in keeping content if it's not used.

Kijewski · 2024-11-20T13:47:33Z

rinja_parser/src/target.rs

+        Target::Name("_") | Target::Placeholder(_) => {
+            return Err(winnow::error::ErrMode::Cut(ErrorContext::new(
+                "reserved keyword `_` cannot be used here",
+                i,
+            )));
+        }


IMHO, this use of placeholders is fine. Maybe I want to {% let _ = iter.next() %} or {% let [_] = slice.try_into().unwrap() %}, because I want to test something in my code, generate an effect, whatever. Sorry, but I think whole function can the removed, because I don't want to restrict what can be skipped on the lhs of an assignment. :-/

Ok, let's go then. :)

GuillaumeGomez · 2024-11-20T13:56:58Z

Ah, we have a new license in our dependency tree apparently.

Kijewski · 2024-11-20T14:01:36Z

deny.toml

-allow = ["Apache-2.0", "MIT", "MIT-0", "Unicode-DFS-2016"]
+allow = ["Apache-2.0", "MIT", "MIT-0", "Unicode-3.0"]


Yeah, the license was changed "about 5 hours ago" :)

Lucky me. :')

Kijewski

Thank you very much for the change! :)

Kijewski reviewed Nov 15, 2024

View reviewed changes

GuillaumeGomez force-pushed the allow-underscore branch from 09e1534 to 05ae1ba Compare November 15, 2024 13:54

GuillaumeGomez force-pushed the allow-underscore branch from 05ae1ba to c2b0db0 Compare November 18, 2024 22:06

cipriancraciun mentioned this pull request Nov 19, 2024

Allow identifiers to start with an uppercase letter #256

Open

Kijewski reviewed Nov 19, 2024

View reviewed changes

Allow idents to start with _

f39a846

GuillaumeGomez force-pushed the allow-underscore branch from c2b0db0 to 3d13a6e Compare November 19, 2024 17:10

Kijewski reviewed Nov 19, 2024

View reviewed changes

GuillaumeGomez added 3 commits November 19, 2024 21:21

Extend checks for underscore ident misuses

7aa8868

Handle .. in is_shadowing_variable as well

5dce4a6

Simplify Target::PlaceHolder handling

53f15d8

GuillaumeGomez force-pushed the allow-underscore branch from 3d13a6e to 53f15d8 Compare November 19, 2024 20:31

GuillaumeGomez requested a review from Kijewski November 19, 2024 20:33

Kijewski reviewed Nov 20, 2024

View reviewed changes

Allow to create variables named _

8dbbb9a

Update license in deny.toml

2ec7740

GuillaumeGomez force-pushed the allow-underscore branch from e160721 to 2ec7740 Compare November 20, 2024 14:00

Kijewski reviewed Nov 20, 2024

View reviewed changes

Kijewski approved these changes Nov 20, 2024

View reviewed changes

GuillaumeGomez merged commit b856eb8 into rinja-rs:master Nov 20, 2024
36 checks passed

GuillaumeGomez deleted the allow-underscore branch November 20, 2024 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow idents to start with `_` #244

Allow idents to start with `_` #244

GuillaumeGomez commented Nov 14, 2024

Kijewski Nov 15, 2024 •

edited

Loading

GuillaumeGomez Nov 15, 2024

Kijewski commented Nov 15, 2024

GuillaumeGomez commented Nov 15, 2024

Kijewski commented Nov 18, 2024

GuillaumeGomez commented Nov 18, 2024

Kijewski commented Nov 18, 2024

GuillaumeGomez commented Nov 18, 2024

Kijewski commented Nov 18, 2024

cipriancraciun commented Nov 18, 2024 •

edited

Loading

GuillaumeGomez commented Nov 18, 2024

GuillaumeGomez commented Nov 18, 2024

Kijewski Nov 19, 2024

GuillaumeGomez Nov 19, 2024

GuillaumeGomez commented Nov 19, 2024

Kijewski Nov 19, 2024

GuillaumeGomez Nov 19, 2024

Kijewski Nov 20, 2024

GuillaumeGomez Nov 20, 2024

GuillaumeGomez commented Nov 20, 2024

Kijewski Nov 20, 2024

GuillaumeGomez Nov 20, 2024

Kijewski left a comment

		allow = ["Apache-2.0", "MIT", "MIT-0", "Unicode-DFS-2016"]
		allow = ["Apache-2.0", "MIT", "MIT-0", "Unicode-3.0"]

Allow idents to start with _ #244

Allow idents to start with _ #244

Conversation

GuillaumeGomez commented Nov 14, 2024

Kijewski Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kijewski commented Nov 15, 2024

GuillaumeGomez commented Nov 15, 2024

Kijewski commented Nov 18, 2024

GuillaumeGomez commented Nov 18, 2024

Kijewski commented Nov 18, 2024

GuillaumeGomez commented Nov 18, 2024

Kijewski commented Nov 18, 2024

cipriancraciun commented Nov 18, 2024 • edited Loading

GuillaumeGomez commented Nov 18, 2024

GuillaumeGomez commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kijewski left a comment

Choose a reason for hiding this comment

Allow idents to start with `_` #244

Allow idents to start with `_` #244

Kijewski Nov 15, 2024 •

edited

Loading

cipriancraciun commented Nov 18, 2024 •

edited

Loading