Skip to content

refactor: clarify uppercase exotic ident path #6779

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 30, 2024
Merged

refactor: clarify uppercase exotic ident path #6779

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8026a0
refactor: clarify uppercase exotic ident path
cometkim May 25, 2024
842fcad
remove unnecessary unwrap
cometkim May 26, 2024
b0c6f1e
use `String.sub` intead of `unwrap_uppercase_exotic`
cometkim May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions jscomp/syntax/src/res_printer.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ let print_longident = function
| Longident.Lident txt -> Doc.text txt
| lid -> Doc.join ~sep:Doc.dot (print_longident_aux [] lid)

type identifier_style = ExoticIdent | NormalIdent
type identifier_style = UppercaseExoticIdent | ExoticIdent | NormalIdent

let classify_ident_content ?(allow_uident = false) ?(allow_hyphen = false) txt =
if Token.is_keyword_txt txt then ExoticIdent
Expand All @@ -388,9 +388,9 @@ let classify_ident_content ?(allow_uident = false) ?(allow_hyphen = false) txt =
if i == len then NormalIdent
else if i == 0 then
match String.unsafe_get txt i with
| '\\' -> UppercaseExoticIdent
| 'A' .. 'Z' when allow_uident -> loop (i + 1)
| 'a' .. 'z' | '_' -> loop (i + 1)
| '-' when allow_hyphen -> loop (i + 1)
| _ -> ExoticIdent
else
match String.unsafe_get txt i with
Expand All @@ -401,10 +401,9 @@ let classify_ident_content ?(allow_uident = false) ?(allow_hyphen = false) txt =
loop 0

let print_ident_like ?allow_uident ?allow_hyphen txt =
let txt = Ext_ident.unwrap_uppercase_exotic txt in
match classify_ident_content ?allow_uident ?allow_hyphen txt with
| ExoticIdent -> Doc.concat [Doc.text "\\\""; Doc.text txt; Doc.text "\""]
| NormalIdent -> Doc.text txt
| UppercaseExoticIdent | NormalIdent -> Doc.text txt

let rec unsafe_for_all_range s ~start ~finish p =
start > finish
Expand Down Expand Up @@ -435,8 +434,12 @@ let print_poly_var_ident txt =
(* numeric poly-vars don't need quotes: #644 *)
if is_valid_numeric_polyvar_number txt then Doc.text txt
else
let txt = Ext_ident.unwrap_uppercase_exotic txt in
match classify_ident_content ~allow_uident:true txt with
| UppercaseExoticIdent ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cases unwraps then re-wraps the ident.
What's the net effect? Just to remove \ ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does.

Maybe we can replace it to

      let len = String.length txt in
      Doc.text ((String.sub [@doesNotRaise]) txt 1 (len - 1))

Copy link
Collaborator

@cristianoc cristianoc May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that, or add a comment that that's the effect, is fine.
Just, the current code can be a bit confusing without additional explanation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I don't know if it is possible, but one could try to classify the ident content already at scanning time, instead of print time.
And any id that is classified to be exotic could be represented internally as "id".
So the printer would need to do no special work.
And uppercase exotic ids would just be exotic ids that happen to start with capital letter -- nothing special.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had tried it within a previous PR to get as much work done from the scanner as possible, but it ultimately failed. I have no other ideas on how to do it while still using the existing parsetree as is. What do you mean "id"? Is there another place to mark it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually never mind, the scanner does not have the context that the printer has, such as when an uppercase identified is allowed.
Good to go for me.

let len = String.length txt in
(* UppercaseExoticIdent follows the \"..." format,
so removing the leading backslash is enough to transform it into polyvar style *)
Doc.text ((String.sub [@doesNotRaise]) txt 1 (len - 1))
| ExoticIdent -> Doc.concat [Doc.text "\""; Doc.text txt; Doc.text "\""]
| NormalIdent -> (
match txt with
Expand Down
Loading