Remove one "impossible" error by forki · Pull Request #5746 · dotnet/fsharp

forki · 2018-10-09T07:20:23Z

matthid · 2018-10-09T07:27:04Z

But this case is impossible, correct? Shouldn't we report this somehow?

"impossible"

What I mean is that item is matched some lines above to be one of the two cases and item is immutable? This case means someone has changed something and missed to do some work. Removing failwith means the mistake will be spotted later (or not at all)

actually that is what already happend. the case was added and nobody changed the failwith "impossible"

But line above reads | (Item.UnionCase _ | Item.ExnCase _) as item -> how can this match another case?

it does not today. wait until someone adds the active pattern part ;-)

matthid · 2018-10-09T07:28:45Z

Should we still report that this error message might need to be refined (in case it happens)? Would this be an advantage of failwith "impossible" here?

failwith "impossible" is arguably the worst you can do

Yes ideally the code would be written in a way to not compile when written incorrectly, but if that cannot be achieved imho throwing is better than doing/reporting potentially incorrect messages

Actually I think even having a confusing error message is better than what existed. The current state of the world (without @forki's changes) paints the whole editor red if you use the incorrect code:

These internal errors are critical to root out given this, since it's pretty much undiagnosable where the user error is.

Ok but imho we still should make sure the error message indicates this is not implemented and something which needs to be fixed if it occurs.
Otherwise users get wrong messages on their code which is arguably worse than a compiler crash

we don't give a wrong message. We just give a very generic one

forki · 2018-10-09T12:06:27Z

@KevinRansom is something wrong with master? the error for linux build seems unrelated

KevinRansom · 2018-10-09T14:27:56Z

@forki not that I know. The error is:

2018-10-09T12:23:11.3515703Z   /home/vsts/work/1/s/src/scripts/scriptlib.fsx(65,25): error FS0041: A unique overload for method 'GetFileName' could not be determined based on type information prior to this program point. A type annotation may be needed. Candidates: Path.GetFileName(path: ReadOnlySpan<char>) : ReadOnlySpan<char>, Path.GetFileName(path: string) : string [/home/vsts/work/1/s/tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.fsproj]
2018-10-09T12:23:11.3517355Z   /home/vsts/work/1/s/src/FSharpSource.targets(208,5): error MSB3073: The command "mono /home/vsts/work/1/s/packages/FSharp.Compiler.Tools.4.1.27/tools/fsi.exe --exec "/home/vsts/work/1/s/src/scripts/subst.fsx" --in:"/home/vsts/work/1/s/tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.dll.config" --out:"/home/vsts/work/1/s/tests/FSharp.Core.UnitTests/../../src/../release/net40/bin/FSharp.Core.UnitTests.dll.config" --pattern1:"FSCoreVersion" --replacement1:"4.5.0.0" --pattern2:"" --replacement2:"" " exited with code 1. [/home/vsts/work/1/s/tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.fsproj]
2018-10-09T12:23:11.3517711Z 
2

At this point I believe the script is running the built compiler, so I suppose there is some possibility they are related.

cartermp · 2018-10-09T14:47:38Z

@KevinRansom #5733 also has this error but with no compiler, so something's wrong in the linux build

KevinRansom · 2018-10-09T14:49:56Z

@cartermp indeed it does.

forki · 2018-10-10T12:04:30Z

it's noew green after rebase

forki force-pushed the i5745 branch from 3f30603 to 7c30534 Compare October 9, 2018 07:26

matthid reviewed Oct 9, 2018

View reviewed changes

forki force-pushed the i5745 branch 4 times, most recently from 423564f to 89e498b Compare October 9, 2018 12:06

forki added 4 commits October 10, 2018 12:21

Remove one "impossible" error - fixes dotnet#5745

2383ff0

Fix tests

c256b80

Add test for dotnet#5745

05ec633

Fix "build from source"

7a23ccf

forki force-pushed the i5745 branch from 89e498b to 7a23ccf Compare October 10, 2018 10:21

dsyme approved these changes Oct 12, 2018

View reviewed changes

cartermp approved these changes Oct 12, 2018

View reviewed changes

KevinRansom merged commit 39fd7b8 into dotnet:master Oct 15, 2018

Conversation

forki commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forki commented Oct 9, 2018

Uh oh!

KevinRansom commented Oct 9, 2018

Uh oh!

cartermp commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom commented Oct 9, 2018

Uh oh!

forki commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

forki commented Oct 9, 2018 •

edited

Loading

cartermp commented Oct 9, 2018 •

edited

Loading