Use raw identifiers for identifiers which collide with Rust keywords

[SciStarter]

Rust keywords are valid identifier names in templates, so allow the template expansions to use those identifiers safely, by turning them into raw identifiers during the parse stage.

[vallentin]

Unrelated, but why are you posting as @SciStarter, but committing as @djarb?

[SciStarter]

I'm doing this for work, but actually editing the code on my personal computer.

…

On Mon, Apr 19, 2021, 10:36 Christian Vallentin ***@***.***> wrote: Unrelated, but how come you're posting as @SciStarter <https://github.com/SciStarter>, but committing as @djarb <https://github.com/djarb>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#476 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANP35AYEG56Q6NZBDJXD2NTTJRS3PANCNFSM43GIJ3LQ> .

[djc]

Why do this in the parser? It would make much more sense to me in the code generator.

[SciStarter]

In the parser, there's a single point where all identifiers are handled, so I could make the change there with minimal code impact. From that point, the identifiers fan out into many parts of the code. It's hard to be certain of finding all the places that would need to be changed if the renaming was deferred. Besides which, a case can be made that the parser is the right place to make this change. It's a Nom parser, so it doesn't actually have a tokenizer, but the tokenizer would be a very normal place to do normalization of alternate representations of a word.

…

On Mon, Apr 19, 2021 at 11:41 AM Dirkjan Ochtman ***@***.***> wrote: Why do this in the parser? It would make much more sense to me in the code generator. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#476 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANP35A3RGLXGPAXB7O7ETZLTJR2MHANCNFSM43GIJ3LQ> .

-- Daniel Arbuckle, Ph. D. Bespoke Software, Data Analysis, Consulting, Web Apps ***@***.*** (509) 449-2745

[djc]

I'm sorry, but that doesn't make much sense to me. The parser is supposed to parse text into an AST that represents the template as parsed in a way that is easier to handle. The exact name of the variables only becomes a problem when they're used in the generated Rust code -- so that's where they should be renamed. I'm pretty sure there's only a few places where the code generator generates code for variable references; you could start by looking here.

(Since we're generating code into a buffer here, I don't think we need parallel arrays with r#-prefixed keywords. Just one array of keywords is enough, and then the code generator should take care of pushing the prefix into the buffer where needed.)

[SciStarter]

Okay, I've moved the identifier normalization into the code generator. Is this more to your liking?

[djc]

This is much better, thanks! As I understand it, though, we don't actually need to store the raw form in the generator's data structures -- we only need to put the raw form in the generated code, right? That should simplify this some more and also make it easier to follow through on my suggestion from the last comment that we don't need the two separate arrays.

Also, please put the constants and helpers near the bottom of the module as per the convention in this project.

[SciStarter]

Unfortunately, that doesn't work due to the way expressions are handled (specifically, because they're rendered into a returned string instead of directly into the output buffer). I'd have to make extensive changes to the expression processing.

[SciStarter]

However, due to the return value actually being a String, I can actually do something reasonable along those lines after all.

[vallentin]

Ah, I see what the issue is. It's because the key in locals: MapChain<...> is a &str and not a String. A workaround could then be to change the key to a String or Cow<'a, str>. Then it would be possible for normalize_identifier() to return Cow<'a, str> instead.

The question now ofc is, whether the price of dynamically allocated keys is better or worse, than the increased binary size, by having the "duplicate" arrays.

[SciStarter]

As a rule of thumb, I prefer increased size over slower execution, but that's just a general opinion. Do you have a preference in this instance?

[djc]

I think I prefer the constants, but let's make it one array of tuples rather than parallel arrays (and let rustfmt do its thing on it).

[djc]

Thanks for doing the work on this!

[SciStarter]

Thanks for being open to the idea :)