-
Notifications
You must be signed in to change notification settings - Fork 833
Do not use Unicode aware API and CurrentCulture in compiler #16066
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
Changes from all commits
45ede26
bd9461c
539be60
7f7f3fd
4bd39eb
60da597
9739f99
b4afa99
ea33585
4325d8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3756,7 +3756,7 @@ let writePdb ( | |
let pdbfileInfo = FileInfo(pdbfile).FullName | ||
|
||
// If pdbfilepath matches output filepath then error | ||
if String.Compare(outfileInfo, pdbfileInfo, StringComparison.InvariantCulture) = 0 then | ||
if outfileInfo = pdbfileInfo then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the particular motivation for this one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. File systems usually doesn't normalize Unicode in paths, InvariantCulture does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those paths are CLI arguments, the logic to compare them should stay case insensitive like it was, to keep the behavior the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @T-Gro it was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @T-Gro do you suggest to actually make it case insensitive? |
||
errorR(Error(FSComp.SR.optsPdbMatchesOutputFileName(), rangeStartup)) | ||
try FileSystem.FileDeleteShim pdbfile with _ -> () | ||
use fs = FileSystem.OpenFileForWriteShim(pdbfile, fileMode = FileMode.Create, fileAccess = FileAccess.ReadWrite) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,13 +140,13 @@ module internal Parse = | |
let rec digitsPrecision (fmt: string) (fmtPos: int) = | ||
if fmtPos >= fmt.Length then failwith (FSComp.SR.forBadPrecision()) | ||
match fmt[fmtPos] with | ||
| c when System.Char.IsDigit c -> digitsPrecision fmt (fmtPos+1) | ||
| c when isDigit c -> digitsPrecision fmt (fmtPos+1) | ||
| _ -> fmtPos | ||
|
||
let precision (info: FormatInfoRegister) (fmt: string) (fmtPos: int) = | ||
if fmtPos >= fmt.Length then failwith (FSComp.SR.forBadWidth()) | ||
match fmt[fmtPos] with | ||
| c when System.Char.IsDigit c -> info.precision <- true; false,digitsPrecision fmt (fmtPos+1) | ||
| c when isDigit c -> info.precision <- true; false,digitsPrecision fmt (fmtPos+1) | ||
| '*' -> info.precision <- true; true,(fmtPos+1) | ||
| _ -> failwith (FSComp.SR.forPrecisionMissingAfterDot()) | ||
|
||
|
@@ -161,14 +161,14 @@ module internal Parse = | |
let rec go pos n = | ||
if pos >= len then failwith (FSComp.SR.forBadPrecision()) | ||
match fmt[pos] with | ||
| c when System.Char.IsDigit c -> go (pos+1) (n*10 + int c - int '0') | ||
| c when isDigit c -> go (pos+1) (n*10 + int c - int '0') | ||
| _ -> Some n, optionalDotAndPrecision info fmt pos | ||
go fmtPos intAcc | ||
|
||
let widthAndPrecision (info: FormatInfoRegister) (fmt: string) (fmtPos: int) = | ||
if fmtPos >= fmt.Length then failwith (FSComp.SR.forBadPrecision()) | ||
match fmt[fmtPos] with | ||
| c when System.Char.IsDigit c -> false,digitsWidthAndPrecision info fmt fmtPos 0 | ||
| c when isDigit c -> false,digitsWidthAndPrecision info fmt fmtPos 0 | ||
| '*' -> true, (None, optionalDotAndPrecision info fmt (fmtPos+1)) | ||
| _ -> false, (None, optionalDotAndPrecision info fmt fmtPos) | ||
|
||
|
@@ -178,7 +178,7 @@ module internal Parse = | |
let rec digitsPosition n pos = | ||
if pos >= len then failwith (FSComp.SR.forBadPrecision()) | ||
match fmt[pos] with | ||
| c when System.Char.IsDigit c -> digitsPosition (n*10 + int c - int '0') (pos+1) | ||
| c when isDigit c -> digitsPosition (n*10 + int c - int '0') (pos+1) | ||
| '$' -> Some n, pos+1 | ||
| _ -> None, pos | ||
|
||
|
@@ -371,7 +371,7 @@ let parseFormatStringInternal | |
// type checker. They should always have '(...)' after for format string. | ||
let requireAndSkipInterpolationHoleFormat i = | ||
if i < len && fmt[i] = '(' then | ||
let i2 = fmt.IndexOfOrdinal(")", i+1) | ||
let i2 = fmt.IndexOf(')', i+1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not ordinal here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. char overloads are always Ordinal. My PR is larger than #16439 (that introduced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if i2 = -1 then | ||
failwith (FSComp.SR.forFormatInvalidForInterpolated3()) | ||
else | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,7 +58,7 @@ module internal PrintUtilities = | |
let isDiscard (name: string) = name.StartsWithOrdinal("_") | ||
|
||
let ensureFloat (s: string) = | ||
if String.forall (fun c -> Char.IsDigit c || c = '-') s then | ||
if String.forall (fun c -> isDigit c || c = '-') s then | ||
s + ".0" | ||
else s | ||
|
||
|
@@ -925,7 +925,7 @@ module PrintTypes = | |
if not denv.includeStaticParametersInTypeNames then | ||
None, args | ||
else | ||
let regex = System.Text.RegularExpressions.Regex(@"\`\d+") | ||
let regex = System.Text.RegularExpressions.Regex(@"\`\d+", System.Text.RegularExpressions.RegexOptions.ECMAScript) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haven't seen this yet - which of course doesn't mean this is not a valid change - just wondering, is this a recommendation from Microsoft or, in other words, what is it consistent with? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See notes in OP. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a regular expression, this could also just be something like: let tryGetTyparCount (s: string) =
let indexOfBacktick = s.LastIndexOf '`'
if indexOfBacktick >= 0 && indexOfBacktick < s.Length - 1 then
match Int32.TryParse(s.AsSpan(indexOfBacktick + 1)) with
| true, genericArgCount -> ValueSome genericArgCount
| false, _ -> ValueNone
else
ValueNone |
||
let path, skip = | ||
(0, tc.CompilationPath.DemangledPath) | ||
||> List.mapFold (fun skip path -> | ||
|
@@ -1962,7 +1962,7 @@ module TastDefinitionPrinting = | |
not (impliedNames.Contains minfo.DisplayName) && | ||
IsMethInfoAccessible amap m ad minfo && | ||
// Discard method impls such as System.IConvertible.ToBoolean | ||
not (minfo.IsILMethod && minfo.DisplayName.Contains(".")) && | ||
not (minfo.IsILMethod && minfo.DisplayName.Contains('.')) && | ||
not minfo.IsUnionCaseTester && | ||
not (minfo.DisplayName.Split('.') |> Array.exists isDiscard)) | ||
|
||
|
@@ -2440,7 +2440,7 @@ module InferredSigPrinting = | |
| TMDefLet(bind, _) -> | ||
([bind.Var] | ||
|> List.filter filterVal | ||
|> List.map (mkLocalValRef >> PrintTastMemberOrVals.prettyLayoutOfValOrMemberNoInst denv infoReader) | ||
|> List.map (mkLocalValRef >> PrintTastMemberOrVals.prettyLayoutOfValOrMemberNoInst denv infoReader) | ||
|> aboveListL) | ||
|
||
| TMDefOpens _ -> emptyL | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a bug, so I changed
<>
to=
here. Or I did misunderstood something? This was introduced in #13870There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fsharp/src/Compiler/Driver/CompilerImports.fs
Lines 2437 to 2447 in b4afa99
based on calling context this bug was resulting in making any framework assembly as primary assembly equivalent, this may affect some esoteric .NET implementations.