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

Reserved names checking should be case sensitive #646

Closed
andrueastman opened this issue Sep 21, 2021 · 3 comments · Fixed by #657
Closed

Reserved names checking should be case sensitive #646

andrueastman opened this issue Sep 21, 2021 · 3 comments · Fixed by #657
Assignees
Labels
Csharp Pull requests that update .net code type:bug A broken experience

Comments

@andrueastman
Copy link
Member

andrueastman commented Sep 21, 2021

The reserved keywords are in a case insensitive collection at the moment as shown below.

private readonly Lazy<HashSet<string>> _reservedNames = new(() => new(StringComparer.OrdinalIgnoreCase) {

C# has its keywords as case sensitive otherwise this leads to the generation of invalid classnames/file names such as
@Event.cs or a class declaration like public class @Event

Example here

We might have to consider this as well for languages we know for sure are case-sensitive.
AB#11195

@andrueastman andrueastman added the Csharp Pull requests that update .net code label Sep 21, 2021
@baywet
Copy link
Member

baywet commented Sep 21, 2021

Thanks for reporting the issue. One of the reasons why I went with case insensitive is because I didn't want to have duplicated entries (e.g. string and String). But if this is causing issues we could: 1. make the hashset case sensitive. 2. duplicate all the entries that need to be. I think this should be a case by case evaluation for each language. While this doesn't seem to be an issue for typescript. I'm not sure whether it will or not for Go and Ruby.
Would you mind taking this on?

@baywet baywet added the type:bug A broken experience label Sep 21, 2021
@baywet baywet added this to the TypeWriter Replacement milestone Sep 21, 2021
@andrueastman
Copy link
Member Author

No worries @baywet.
I'll also try to do some investigation on Ruby and Go and update this issue on whether we need to go that far for them as well.

@andrueastman
Copy link
Member Author

andrueastman commented Sep 24, 2021

Taking a look at the other languages (Go, Ruby), it seems that the reserved name escaping is done by adding a suffix (the string _escaped) rather than a prefix as is done in .Net. So, I don't think this will affect other languages as language rules want identifiers to start with alphabetical characters.

Go - https://golang.org/ref/spec#Identifiers
Ruby - https://ruby-doc.org/docs/ruby-doc-bundle/Manual/man-1.4/syntax.html#ident

After looking at the code, the approach suggested, might not work as reservedNames are matched with the name of the codeElement (which may already be in lower case to be later converted to the language specific scenario like capitalization)

#657 resolves this by adding the ability to add a blacklist to prevent the reserved names replacement based on the CodeElement type as we may know beforehand that this is unnecessary (e.g a class name that will be capitalized or to prevent the changing of the property value)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code type:bug A broken experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants