Skip to content

Fix incomplete pattern matches over C# enums producing the wrong warning - #5206

Merged
KevinRansom merged 5 commits into
dotnet:masterfrom
jwosty:warning-fs0104-fix
Jul 13, 2018
Merged

Fix incomplete pattern matches over C# enums producing the wrong warning#5206
KevinRansom merged 5 commits into
dotnet:masterfrom
jwosty:warning-fs0104-fix

Conversation

@jwosty

Copy link
Copy Markdown
Contributor

PR #4522 has a bug, causing it to always produce FS0104 for non-F#-defined enum. For example, the following code:

// System.DateTimeKind = | Unspecified = 0 | Utc = 1 | Local = 2letf=function System.DateTimeKind.Unspecified ->0

currently produces this compiler warning:

typecheck error FS0104: Enums may take values outside known cases. For example, the value 'enum<System.DateTimeKind> (1)' may indicate a case not covered by the pattern(s).

when it is supposed to produce:

typecheck error FS0025: Incomplete pattern matches on this expression. For example, the value 'System.DateTimeKind.Utc' may indicate a case not covered by the pattern(s).

This PR corrects the issue.

Thanks to @charlesroddiefor identifying the bug.

@jwostyjwosty changed the title Warning FS0104: Fix Fix incomplete pattern matches over C# enums producing the wrong warningJun 16, 2018
@jwosty

Copy link
Copy Markdown
ContributorAuthor

@dotnet-bot test this please

| PathEmpty(_), PathEmpty(_) -> true
| _ -> false

//// (Temporarily copy-pasted from TypeChecker.fs)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I don't know where this function should be refactored to as I'm not very well versed in the compiler's file structure. PatternMatchCompilation.fs comes before TypeChecker.fs, but the former needs this function.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(just an opinion) You can expose it for use in TypeChecker.fs, and replace the comment akin to "Comes from TypeChecker.fs, might move to more apropriate place later.".

This would be better than having the code duplicated, and unless it becomes used in a third place, the location is not too important.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@smoothdeveloper I kind of like that idea actually. That way its pretty obvious that not much code depends on it.

@jwosty

Copy link
Copy Markdown
ContributorAuthor

@cartermp What is the cause of the CI failure? It reports:

Exception: Yaaf.FSharp.Scripting.FsiEvaluationException: Error while compiling or executing fsharp snippet. ---> System.Exception: Operation failed. The error text has been printed in the error stream. To return the corresponding FSharpErrorInfo use the EvalInteractionNonThrowing, EvalScriptNonThrowing or EvalExpressionNonThrowing
... etc etc
System.Net.WebException: The remote server returned an error: (404) Not Found.
at System.Net.WebClient.DownloadFile(Uri address, String fileName

@cartermp

Copy link
Copy Markdown
Contributor

cc @matthid - sounds like a FAKE issue on CI?

@matthid

Copy link
Copy Markdown
Contributor

@forki Is this the broken .NET SDK installer problem? Where is the other issue about that?

@forki

forki commented Jun 21, 2018 via email

Copy link
Copy Markdown
Contributor

@cartermp

Copy link
Copy Markdown
Contributor

test Ubuntu16.04 Release_fcs Build Windows_NT Release_fcs Build

@cartermp

Copy link
Copy Markdown
Contributor

test Windows_NT Release_fcs Build

@jwosty

Copy link
Copy Markdown
ContributorAuthor

Thanks @cartermp! Now that CI is working... Requesting reviews 🙂 @dsyme@forki

@matthid

Copy link
Copy Markdown
Contributor

So typical try again until it works error?

@cartermp

Copy link
Copy Markdown
Contributor

@matthid jawohl!

@jwosty

Copy link
Copy Markdown
ContributorAuthor

There's no more for me to do at the moment. Could this be considered for merge? @dsyme

@KevinRansom

Copy link
Copy Markdown
Contributor

Thanks for this

@KevinRansom
KevinRansom merged commit 3e53374 into dotnet:masterJul 13, 2018
@jwosty
jwosty deleted the warning-fs0104-fix branch July 21, 2023 16:49
@jwosty
jwosty restored the warning-fs0104-fix branch July 21, 2023 16:49
@jwosty
jwosty deleted the warning-fs0104-fix branch July 21, 2023 16:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@jwosty@cartermp@matthid@forki@KevinRansom@smoothdeveloper@dsyme