Skip to content

Conversation

@abelbraaksma
Copy link
Contributor

@abelbraaksma abelbraaksma commented Jul 25, 2020

UPDATE: continued here: #9906 (had to close this because Github messed with my commits somehow)

Fixes #3487.
Fixes #9787.

This resurrects the work done in #5498 by @misonijnik . In short, it's an attempt to make sure that:

#iff FALSE   // raises wrong error, this should fix that (should be invalid token error)
#foo "test" // doesn't raise error at all, but should (invalid token)
#foo 42   // raises wrong error, this should fix that (should be invalid token error)

module X =
    #foo 42   // shows warning, but should be error (invalid token)

Some tests were failing on the original PR, I merged it with latest master and marked this WIP, as I want to see what those errors were, though I don't expect to work long on it, the change is pretty benign ;).

List of fixes (will be updated if necessary)

  • Fix wrong hash-token parsing (partially done)

    • with the original change by @misonijnik, # is now only recognized and leading to tokens if it is valid. This should be updated to include invalid items and the proper error and range for the lang service.
  • Allow a type to start with the letters "if"

    Previously
    image

    Implemented:
    image

  • Fix wrong error for use of #indent (text mentions '#light', instead of '#indent') (FS0000 warning (not assigned) for #light or #indent being misplaced #9787):
    image

    Implemented, whole token shown:
    image

  • Fix wrong error for use of #light (FS0000 warning (not assigned) for #light or #indent being misplaced #9787):
    image

    Implemented:
    image

  • Fix wrong error when using wrong argument with #light:
    image

  • Improve #light when token is wrong:
    image

    After @misonijnik's change (error and range still to improve):
    image

  • Fix wrong error with wrong token with #if:
    image

    After @misonijnik's change (error and range still to improve):
    image

  • This fsharpqa test explicitly tests for leniency with hash-types starting with #light: Conformance\TypesAndTypeConstraints\TypeParameterDefinitions (HashConstraint02.fs).

    // Regression test for FSHARP1.0:1419
    // Tokens beginning with # should not match greedily with directives
    // The only case where we are still a bit confused is #light: for this reason the code
    // below compiles just fine (it would not work if I replace #light with #r for example)
    #light
    
    type light_() = class
                    end
    
    let t = new light_()
    
    let t5 (x : #light_) = x       // this is now valid, and does not throw a warning
    
    let r1 = t5 1.0         // this is now INVALID! (as it should)
    let r2 = t5 t
    
    exit 0
    

    Current situation
    image

    New situation
    image

  • Fix wrong parsing of light when used as hash-constraint:

    Current situation
    image

Generally: when # is followed by an unrecognized token, I would expect an error that says that the token is not recognized.

TODO:

  • Fix failing build
  • Verify & improve errors / warnings
  • add tests
  • Review / improve code changes

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Jul 25, 2020

This is bound to be a painful exercise in fixing the failing test: it's one of those "works locally, but not in CI".

I ran it locally in a VS running the updates (admittedly I'm not sure if this extends to the test runner, so I may be seeing a false positive, @cartermp, can you (un)confirm running VS from VisualFSharp.sln using VisualFSharpFull build and Ctrl+F5 runs tests with all updates? @vzarytovskii, you've been doing a lot with tests, do you know how I should run the test BuildAndClean locally so that it fails like in this PR in CI and I can debug/analyze?)

EDIT 1: the build settings in the dynamic fsproj file point correctly to artifacts\bin\VisualFSharp.UnitTests\Debug\net472\Microsoft.FSharp.targets and inside it, MSBuildThisFileDirectory is used, so it appears the settings are correct and the test runs using the local build, even from VS Tests window.

EDIT 2: with whatever build settings I try, I still cannot get this test to fail locally.

image

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Jul 26, 2020

Still no idea what happens here, I can't repro it locally (yet). But I won't give up 😄 .

deviousasti and others added 9 commits July 27, 2020 14:13
There exists no method to retrieve the original source string because the 'String' property on StringText is never exposed in ISourceText
This being a container for a source string, while identical StringTexts will  have the same GetHashCode(), it fails for equality testing because a StringText object is compared to the other's source text, which is always false.

The comparison to string behavior is retained.
Fix Equality comparison for `StringText`
remove version numbers from product details strings
The NetSdk.targets had a duplicate `CollectFSharpDesignTimeTools` target, and so now they don't.
@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Jul 29, 2020

Definitely a case of being fixated on the wrong axiom that "existing tests are always right". I totally missed this glaring error that has been around in this test for a while:

                File.AppendAllText(fooPath, "#light")
                File.AppendAllText(fooPath, "module Foo")

This produces a file with contents #lightmodule Foo, which certainly wasn't the intention of this test. The only reason it passed was that the tokenizer wrongly tokenized #light and #if before this change, which allowed #lightmodule Foo to (incorrectly) compile with a warning.

And I just read these lines a million times, never bothered to check the output. Until I did ;).

Changed it to output the intended two lines of code instead of one malformed line.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Jul 29, 2020

And the reason that it "succeeded locally, but not remotely"? In case you're curious: running build.cmd doesn't always build everything, and when you switch from branch to branch in your solution, the lex.fsl file does not always get recompiled (certainly not always from Visual Studio, it appears).

These are things that may be fixed someday to get a cleaner experience. Until then, I should just be more careful.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Jul 29, 2020

Two failing tests, E_MustBeIdent02.fs and HashConstraint02.fs. Both these tests use #xyz style hash-constraints, where xyz starts with "if" or "light", and these tests were wrong as they were not parsed as flexible type constraints.

Previous situation:
image

New situation:
image

Original (wrong) tests E_MustBeIdent02.fs:

https://dev.azure.com/dnceng/public/_build/results?buildId=749871&view=logs&j=f63a89e5-f92e-5427-3260-3ba3a585220c&t=bba6f3d9-442e-575e-b64e-01cda67a537e&l=3117

// #Regression #Conformance #LexicalAnalysis
// Regression test for FSHARP1.0:1419
//<Expects id="FS1169" span="(7,13-7,16)" status="error">#if directive should be immediately followed by an identifier</Expects>
//<Expects id="FS0010" span="(8,12-8,19)" status="error">#endif has no matching #if in pattern</Expects>
//<Expects id="FS0583" span="(8,8-8,9)" status="error">Unmatched '\('</Expects>
#light
let t8 (x : #if_) = ()             // should throw type error only
let t7 (x : #endif_) = ()         // should throw type error only

exit 1

dotnet-maestro bot and others added 4 commits July 29, 2020 10:55
…724.1

Microsoft.DotNet.Arcade.Sdk
 From Version 5.0.0-beta.20364.3 -> To Version 5.0.0-beta.20374.1
* [WIP] Add execution output support to test framework

* Added output for in-process exection via redirecting stderr+stdout + catching excepetions; Added output for out-of-process execution via pinfo; Addded Test framework functions to assert stderr/stdout/exit code

* Move CodeQuotationTests to the FSharp.Compiler.ComponentTests
…4516-a6e9-ed86886bbf3e

[master] Update dependencies from dotnet/arcade
* Prevent assignment to literal ILFields

* revert old mechanism

* add new error message, use it, and provide localizations

* add error message for literal and non-literal assignment

* first stab at tests

* flip directory separator in test project file

* fix namespace on file to allow for it to be picked up

* Update src/fsharp/TypeChecker.fs

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Update tests/FSharp.Compiler.ComponentTests/Interop/SimpleInteropTests.fs

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Update tests/FSharp.Compiler.ComponentTests/Interop/SimpleInteropTests.fs

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

Co-authored-by: Phillip Carter <pcarter@fastmail.com>
KevinRansom and others added 2 commits August 3, 2020 16:35
* Implement `nameof x` as a constant pattern

* Resolve ident to find `nameof` in patterns

* fix build

* fix build

* re-enable tests

* fix test

* fix operators

* align code

* test and fix pattern matching

* fix 8661, 7416

* fix tests and error messages

* 'fix test'

* 'fix test'

* add message for C# and old compilers

* fix build

* suppress error 3501 when a compiler supports nameof

Co-authored-by: Don Syme <dsyme@microsoft.com>
@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 4, 2020

That fixed it, finally :)

Next one: Conformance\TypesAndTypeConstraints\TypeParameterDefinitions (HashConstraint02.fs) -- failed (and last one in that log https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/756610/logs/89)


image

This is a false positive!

@cartermp, or anybody, there's a failing test, why did CI succeed? That seems odd (and a bit dangerous).

EDIT, fyi, the log had this:

2020-08-03T18:57:16.0635380Z Conformance\TypesAndTypeConstraints\TypeParameterDefinitions (HashConstraint02.fs) -- failed

and (https://dev.azure.com/dnceng/public/_build/results?buildId=756610&view=logs&j=f63a89e5-f92e-5427-3260-3ba3a585220c&t=bba6f3d9-442e-575e-b64e-01cda67a537e&l=4076):

2020-08-04T17:13:06.9336367Z ##[error](NETCORE_ENGINEERING_TELEMETRY=Test) Failure running tests
2020-08-04T17:13:10.3033902Z ##[error]Cmd.exe exited with code '1'.

but it showed green anyway.

CI telemetry setting a bit too lenient?

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 4, 2020

This is now "really" green, but there are a few things that need improving (errors and ranges, mainly).

cartermp and others added 19 commits August 5, 2020 18:32
…t#9881)

* Fix Package Scripting nuget package

* Compose

* Phillip suggested options should compose too
…otnet#9870)

* Net Standard 2.0 only FSharp.Core (dotnet#9801)

* Net standard only FSharp.Core

* temp

* Cherry-pick of FSharp.Core netstandard + fix for netcoreapp3.1 tests

* Fixed netcoreapp30 -> netcoreapp31 test references

* Cleaned up projects, added testplatform package, moved some tests to new suite

* Tests changes

* Revert accidental revert

* Added missing tests

* Cleaned up packages, fixed tests

* Add System.Runtime to references

Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
* various doc improvements

* don't use uppercase since in cref it is shown as text
* string interploation implementation

* string interploation tests

* escape {{ }}, test verbatim and triple quote, implement .NET specifiers

* fix tests

* string interpolation tests: internal representation corner cases

* string-interp tests should have --langversion:preview

* string interop tests: sprintf

* string interp tests: format specifier negative cases

* string interp tests: format specifier negative cases, .NET-style padding

* fix nested interp strings

* style cleanup

* lex: unify string interp stack and counter

* string-interp: add test cases

* fix mixed quote nested string interpolation

* string-interp: add test case for multiple interpolation points with different indentation

* lexfilter: push new CtxtParen at endPos for INTERP_STRING_PART and INTERP_STRING_BEGIN_PART

* lexfilter: do not check undentation limit for string interpolation tokens.

* FormattableString prototype

* add FormattableString support

* negative error checking

* remove diagnostics

* simpler FormattableString implementation

* fix test

* add testing for nested

* add IFormattable support

* tweak error message

* tests: StringInterpolation: fix case errors

* fix error message

* check number of values matches

* allow use of format strings with printf and friends

* update baselines

* fix baselines

* add Experimental attributes

* update string interp negative tests

* stringinterp test: add PrintFormat tests

* printf: fix empty interpolation string evaluates to null in printf env

* enable test corectly

* Revert "printf: fix empty interpolation string evaluates to null in printf env"

This reverts commit 7f39617.

* simplify codegen for interpolated strings

* fix build

* fix build

* Merge master to feature/string-interp (dotnet#9580)

* Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#9577)

Microsoft.DotNet.Arcade.Sdk
 From Version 1.0.0-beta.20302.3 -> To Version 1.0.0-beta.20326.2

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve perf for String.filter up to 3x (dotnet#9509)

* Improve perf for String.filter 2-2.5x

* Cleanup: remove "foo" etc in tests

* Add tests for new execution path for LOH in String.filter

* Change test string

* String map performance improvement (dotnet#9470)

* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Simplify and improve perf of String.length"

* Resolves dotnet#9470 (comment)

* Lingering space

* Change `String` to use `new` to clarify use of ctor

* Add some better tests for String.map, add side-effect test

* Add tests to ensure the mapping function is called a deterministically amount of times

* Fix typo

* Remove "foo" from String.map tests

* Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement (dotnet#9512)

* Turn String.replicate from O(n) into O(log(n))

* Cleanup String.replicate tests by removing usages of "foo"

* String.replicate: add tests for missing cases, and for the new O(log(n)) cut-off points

* Improve String.replicate algorithm further

* Add tests for String.replicate covering all lines/branches of algo

* Fix accidental comment

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Re enable tests for operators: OperatorsModule1.fs and OperatorsModule2.fs (dotnet#9516) (dotnet#9589)

* Re-enabling tests from OperatorsModule1/2.fs (compile errors)

* Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment.

* Fix `tanh` test, ensure stable result on x86 vs x64 runtimes

* Stop using exception AssertionException, so that test window shows useful info

* Whitespace cleanup and redundant code removal

* Cleanup spelling etc

* Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests

* Special-case floating-point assertion messages for higher precision output

* Fix/update/add tests (some still failing)

* Separate Checked tests, add & fix others, differentiate framework/bitness for some tests

* Add branch for .NET Native (ignore cos test)

* Resorting to comparing floats with a delta using Assert.AreNearEqual

* Add some more tests

Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9599)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9604)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Merge master to feature/string-interp (dotnet#9615)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Merge master to feature/string-interp (dotnet#9619)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

* Text tweeks

* don't auto-resolve types from System.Runtime.WindowsRuntime (dotnet#9644) (dotnet#9648)

Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>

* yeet (dotnet#9657) (dotnet#9661)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* yeet (dotnet#9657) (dotnet#9670)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* fix up tokenizer tests

* fix code review things

* fix code review things

* fix code review things

* fix code review things

* add various testing

* correct continuations for interpolated strings

* fix lexer continuations and colorization for multi-line interpolated strings

* revert xlf changes

* fix assert

* completion and brace matching (not all tests passing yet)

* Fix rebuild

* fix various niggles and get tests working

* fix printf when '%a' in final position

* fix test case

* interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix build

* fix missing error message

* fix % specifiers for interpolated strings

* fix % specifiers for interpolated strings

* fix FCS tests

* minor nits from code review

* code review feedback and use struct tuples in more places

* revert struct tuples

* use struct tuples where possible, byrefs for index

* fix byref for index

* fix ksprintf block size

* make recent cache entry more explicit (cleanup)

* improve performance

* remove unused code

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839) (dotnet#9848)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

* fix dotnet#9893

* fix unmantched right brace in interp string

Co-authored-by: Yatao Li <yatli@microsoft.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
* initial version

* finish porting tests, last test fails

* fix test

* fix end tag

* use cross-framework implementation of endsWithDirectorySeparator

* UsingTask instead of fully-qualified names
@abelbraaksma
Copy link
Contributor Author

No idea why this suddenly says "304 files changed" when clearly (see branch itself) only 7 files changed w.r.t. latest master.

@abelbraaksma
Copy link
Contributor Author

No idea why this suddenly says "304 files changed" when clearly (see branch itself) only 7 files changed w.r.t. latest master.

Still no idea. By just closing this and, using the exact same branch with no extra changes, reopened it as a new PR and that "just worked": #9906.

It's beyond me why I'm seeing 56 commits and 304 changes here, and in the new PR, based on same branch, the correct 21 commits and 7 changed files. Sorry for the mess and noise, just not sure what's going on here...

Sign up for free to 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.

FS0000 warning (not assigned) for #light or #indent being misplaced Invalid directives are not a syntax error, unless in specific places