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

AddBackticksToIdentifierIfNeeded adds ticks for not word. #12303

Open
nojaf opened this issue Oct 26, 2021 · 7 comments
Open

AddBackticksToIdentifierIfNeeded adds ticks for not word. #12303

nojaf opened this issue Oct 26, 2021 · 7 comments
Labels
Area-LangService-API Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Regression
Milestone

Comments

@nojaf
Copy link
Contributor

nojaf commented Oct 26, 2021

Please provide a succinct description of the issue.

Repro steps

Local fsi:

#r @"C:\Users\fverdonck\Projects\fsharp\artifacts\bin\FSharp.Core\Debug\netstandard2.0\FSharp.Core.dll"
#r @"C:\Users\fverdonck\Projects\fsharp\artifacts\bin\FSharp.Compiler.Service\Debug\netstandard2.0\FSharp.Compiler.Service.dll"

FSharp.Compiler.Syntax.PrettyNaming.AddBackticksToIdentifierIfNeeded "not"
|> printfn "FSharp.Compiler.Syntax.PrettyNaming.AddBackticksToIdentifierIfNeeded :  %s"

dotnet artifacts\bin\fsi\Debug\net5.0\fsi.dll C:\Users\fverdonck\Projects\scripts\fsharp\ast-from-local-compiler.fsx

Expected behavior

In previews version of FCS, no ticks are added.

Actual behavior

``not``

Known workarounds

Provide a description of any known workarounds.

Related information

Provide any related information (optional):

FCS 41.0.0-preview.21472.3

@nojaf nojaf added the Bug label Oct 26, 2021
@nojaf
Copy link
Contributor Author

nojaf commented Oct 26, 2021

params and parallel should get ticks and do not get them.

@vzarytovskii
Copy link
Member

Hm, interesting, it got changed in #12072, keywordsWithDescription was moved from the lexhelp to the PrettyNaming module, as a lookup table when we check whether we need to add backticks and not is in it (we didn't use it there prior to this change):

And here's the new logic by which we check whether we need to wrap it:

let IsIdentifierName (name: string) =
not (keywordLookup.Contains name) &&
not (IsUnencodedOpName name) &&
not (IsUnencodedLegacyOpName name) &&
let nameLen = name.Length
nameLen > 0 &&
IsIdentifierFirstCharacter name.[0] &&
let rec loop i = (i >= nameLen || (IsIdentifierPartCharacter(name.[i]) && loop (i+1)))
loop 1

The lookup in keywordLookup is new.

cc @dsyme

@dsyme
Copy link
Contributor

dsyme commented Oct 27, 2021

Hmmm yes I see, it was incorrect to use "keywordsWithDescription", which is mis-named - some of the entries in that table are not keywords (e.g. not, which is a function in the standard library), and not all keywords are present (e.g. reserved ones like params and parallel).

@KevinRansom KevinRansom added Regression Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. labels Oct 27, 2021
@nojaf
Copy link
Contributor Author

nojaf commented Nov 5, 2021

So a fix would be to renamed keywordsWithDescription and update

let keywordsWithDescription : (string * string) list =
[ "abstract", FSComp.SR.keywordDescriptionAbstract()
"and", FSComp.SR.keyworkDescriptionAnd()
"as", FSComp.SR.keywordDescriptionAs()
"assert", FSComp.SR.keywordDescriptionAssert()
"base", FSComp.SR.keywordDescriptionBase()
"begin", FSComp.SR.keywordDescriptionBegin()
"class", FSComp.SR.keywordDescriptionClass()
"const", FSComp.SR.keywordDescriptionConst()
"default", FSComp.SR.keywordDescriptionDefault()
"delegate", FSComp.SR.keywordDescriptionDelegate()
"do", FSComp.SR.keywordDescriptionDo()
"done", FSComp.SR.keywordDescriptionDone()
"downcast", FSComp.SR.keywordDescriptionDowncast()
"downto", FSComp.SR.keywordDescriptionDownto()
"elif", FSComp.SR.keywordDescriptionElif()
"else", FSComp.SR.keywordDescriptionElse()
"end", FSComp.SR.keywordDescriptionEnd()
"exception", FSComp.SR.keywordDescriptionException()
"extern", FSComp.SR.keywordDescriptionExtern()
"false", FSComp.SR.keywordDescriptionTrueFalse()
"finally", FSComp.SR.keywordDescriptionFinally()
"for", FSComp.SR.keywordDescriptionFor()
"fun", FSComp.SR.keywordDescriptionFun()
"function", FSComp.SR.keywordDescriptionFunction()
"global", FSComp.SR.keywordDescriptionGlobal()
"if", FSComp.SR.keywordDescriptionIf()
"in", FSComp.SR.keywordDescriptionIn()
"inherit", FSComp.SR.keywordDescriptionInherit()
"inline", FSComp.SR.keywordDescriptionInline()
"interface", FSComp.SR.keywordDescriptionInterface()
"internal", FSComp.SR.keywordDescriptionInternal()
"lazy", FSComp.SR.keywordDescriptionLazy()
"let", FSComp.SR.keywordDescriptionLet()
"let!", FSComp.SR.keywordDescriptionLetBang()
"match", FSComp.SR.keywordDescriptionMatch()
"match!", FSComp.SR.keywordDescriptionMatchBang()
"member", FSComp.SR.keywordDescriptionMember()
"module", FSComp.SR.keywordDescriptionModule()
"mutable", FSComp.SR.keywordDescriptionMutable()
"namespace", FSComp.SR.keywordDescriptionNamespace()
"new", FSComp.SR.keywordDescriptionNew()
"not", FSComp.SR.keywordDescriptionNot()
"null", FSComp.SR.keywordDescriptionNull()
"of", FSComp.SR.keywordDescriptionOf()
"open", FSComp.SR.keywordDescriptionOpen()
"or", FSComp.SR.keywordDescriptionOr()
"override", FSComp.SR.keywordDescriptionOverride()
"private", FSComp.SR.keywordDescriptionPrivate()
"public", FSComp.SR.keywordDescriptionPublic()
"rec", FSComp.SR.keywordDescriptionRec()
"return", FSComp.SR.keywordDescriptionReturn()
"return!", FSComp.SR.keywordDescriptionReturnBang()
"static", FSComp.SR.keywordDescriptionStatic()
"struct", FSComp.SR.keywordDescriptionStruct()
"then", FSComp.SR.keywordDescriptionThen()
"to", FSComp.SR.keywordDescriptionTo()
"true", FSComp.SR.keywordDescriptionTrueFalse()
"try", FSComp.SR.keywordDescriptionTry()
"type", FSComp.SR.keywordDescriptionType()
"upcast", FSComp.SR.keywordDescriptionUpcast()
"use", FSComp.SR.keywordDescriptionUse()
"use!", FSComp.SR.keywordDescriptionUseBang()
"val", FSComp.SR.keywordDescriptionVal()
"void", FSComp.SR.keywordDescriptionVoid()
"when", FSComp.SR.keywordDescriptionWhen()
"while", FSComp.SR.keywordDescriptionWhile()
"with", FSComp.SR.keywordDescriptionWith()
"yield", FSComp.SR.keywordDescriptionYield()
"yield!", FSComp.SR.keywordDescriptionYieldBang()
"->", FSComp.SR.keywordDescriptionRightArrow()
"<-", FSComp.SR.keywordDescriptionLeftArrow()
":>", FSComp.SR.keywordDescriptionCast()
":?>", FSComp.SR.keywordDescriptionDynamicCast()
"<@", FSComp.SR.keywordDescriptionTypedQuotation()
"@>", FSComp.SR.keywordDescriptionTypedQuotation()
"<@@", FSComp.SR.keywordDescriptionUntypedQuotation()
"@@>", FSComp.SR.keywordDescriptionUntypedQuotation() ]

Remove not and add params and parallel?

@Happypig375
Copy link
Member

Consider not in when 'T: not struct. Here, not is a keyword.

@dsyme
Copy link
Contributor

dsyme commented Nov 5, 2021

Well, sort of. But in any case for the purposes of backtick escape it's not a keyword.

@nojaf
Copy link
Contributor Author

nojaf commented Apr 12, 2022

@dsyme what was the outcome here again of fixing this?
Move keywordList from lexhelp.fs and use that in PrettyNaming?

let private keywordList =
[ FSHARP, "abstract", ABSTRACT
ALWAYS, "and" ,AND
ALWAYS, "as" ,AS
ALWAYS, "assert" ,ASSERT
ALWAYS, "asr" ,INFIX_STAR_STAR_OP "asr"
ALWAYS, "base" ,BASE
ALWAYS, "begin" ,BEGIN
ALWAYS, "class" ,CLASS
FSHARP, "const" ,CONST
FSHARP, "default" ,DEFAULT
FSHARP, "delegate" ,DELEGATE
ALWAYS, "do" ,DO
ALWAYS, "done" ,DONE
FSHARP, "downcast" ,DOWNCAST
ALWAYS, "downto" ,DOWNTO
FSHARP, "elif" ,ELIF
ALWAYS, "else" ,ELSE
ALWAYS, "end" ,END
ALWAYS, "exception" ,EXCEPTION
FSHARP, "extern" ,EXTERN
ALWAYS, "false" ,FALSE
ALWAYS, "finally" ,FINALLY
FSHARP, "fixed" ,FIXED
ALWAYS, "for" ,FOR
ALWAYS, "fun" ,FUN
ALWAYS, "function" ,FUNCTION
FSHARP, "global" ,GLOBAL
ALWAYS, "if" ,IF
ALWAYS, "in" ,IN
ALWAYS, "inherit" ,INHERIT
FSHARP, "inline" ,INLINE
FSHARP, "interface" ,INTERFACE
FSHARP, "internal" ,INTERNAL
ALWAYS, "land" ,INFIX_STAR_DIV_MOD_OP "land"
ALWAYS, "lazy" ,LAZY
ALWAYS, "let" ,LET(false)
ALWAYS, "lor" ,INFIX_STAR_DIV_MOD_OP "lor"
ALWAYS, "lsl" ,INFIX_STAR_STAR_OP "lsl"
ALWAYS, "lsr" ,INFIX_STAR_STAR_OP "lsr"
ALWAYS, "lxor" ,INFIX_STAR_DIV_MOD_OP "lxor"
ALWAYS, "match" ,MATCH
FSHARP, "member" ,MEMBER
ALWAYS, "mod" ,INFIX_STAR_DIV_MOD_OP "mod"
ALWAYS, "module" ,MODULE
ALWAYS, "mutable" ,MUTABLE
FSHARP, "namespace" ,NAMESPACE
ALWAYS, "new" ,NEW
FSHARP, "null" ,NULL
ALWAYS, "of" ,OF
ALWAYS, "open" ,OPEN
ALWAYS, "or" ,OR
FSHARP, "override" ,OVERRIDE
ALWAYS, "private" ,PRIVATE
FSHARP, "public" ,PUBLIC
ALWAYS, "rec" ,REC
FSHARP, "return" ,YIELD(false)
ALWAYS, "sig" ,SIG
FSHARP, "static" ,STATIC
ALWAYS, "struct" ,STRUCT
ALWAYS, "then" ,THEN
ALWAYS, "to" ,TO
ALWAYS, "true" ,TRUE
ALWAYS, "try" ,TRY
ALWAYS, "type" ,TYPE
FSHARP, "upcast" ,UPCAST
FSHARP, "use" ,LET(true)
ALWAYS, "val" ,VAL
FSHARP, "void" ,VOID
ALWAYS, "when" ,WHEN
ALWAYS, "while" ,WHILE
ALWAYS, "with" ,WITH
FSHARP, "yield" ,YIELD(true)
ALWAYS, "_" ,UNDERSCORE
(*------- for prototyping and explaining offside rule *)
FSHARP, "__token_OBLOCKSEP" ,OBLOCKSEP
FSHARP, "__token_OWITH" ,OWITH
FSHARP, "__token_ODECLEND" ,ODECLEND
FSHARP, "__token_OTHEN" ,OTHEN
FSHARP, "__token_OELSE" ,OELSE
FSHARP, "__token_OEND" ,OEND
FSHARP, "__token_ODO" ,ODO
FSHARP, "__token_OLET" ,OLET(true)
FSHARP, "__token_constraint",CONSTRAINT
]
(*------- reserved keywords which are ml-compatibility ids *)
@ List.map (fun s -> (FSHARP,s,RESERVED))
[ "break"; "checked"; "component"; "constraint"; "continue"
"fori"; "include"; "mixin"
"parallel"; "params"; "process"; "protected"; "pure"
"sealed"; "trait"; "tailcall"; "virtual" ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-LangService-API Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Regression
Projects
Status: New
Development

No branches or pull requests

5 participants