Further enhancements to nameof #8754

Tarmil · 2020-03-19T16:20:16Z

Feature suggestion: https://github.com/fsharp/fslang-design/blob/master/FSharp-5.0/FS-1085-nameof-pattern.md

Implementation
Add tests
Check syntax coloring
Check code completion

dnfclas · 2020-03-19T16:20:29Z

All CLA requirements met.

CartBlanche · 2020-06-01T12:53:32Z

@Tarmil Will you be updating this?

Tarmil · 2020-06-02T15:53:20Z

Yeah I'll get back to this soon, hopefully the conflict shouldn't be too hard to resolve.

dsyme · 2020-06-19T12:37:46Z

I've merged master into this. I'll likely integrate the changes into a final PR for nameof

tests/fsharp/core/nameof/preview/test.fsx

dsyme · 2020-06-19T15:51:51Z

This is now ready - it now contains all the design adjustments for nameof

nameof in patterns
nameof for operators returns the source text
Further testing for nameof on generic parameters

tests/fsharp/core/nameof/preview/test.fsx

KevinRansom · 2020-06-22T19:27:15Z

Error on neg16.fs:


 + neg16.fs(116,9,116,13): typecheck error FS0879: Volatile fields must be marked 'mutable' and cannot be thread-static
diff between [D:\a\1\s\tests\fsharp\typecheck\sigs\neg16.bsl] and [D:\a\1\s\tests\fsharp\typecheck\sigs\neg16.vserr]
line 92
 - neg16.fs(113,9,113,11): typecheck error FS0879: Volatile fields must be marked 'mutable' and cannot be thread-static
 + neg16.fs(130,10,130,11): typecheck error FS0935: Types with the 'AllowNullLiteral' attribute may only inherit from or implement types which also allow the use of the null literal
diff between [D:\a\1\s\tests\fsharp\typecheck\sigs\neg16.bsl] and [D:\a\1\s\tests\fsharp\typecheck\sigs\neg16.vserr]
diff at line 92
- 
- neg16.fs(116,9,116,13): typecheck error FS0879: Volatile fields must be marked 'mutable' and cannot be thread-static
- 
- neg16.fs(130,10,130,11): typecheck error FS0935: Types with the 'AllowNullLiteral' attribute may only inherit from or implement types which also allow the use of the null literal
"
  Stack Trace:
     at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1639.Invoke(String message) in D:\a\1\s\src\fsharp\FSharp.Core\printf.fs:line 1639
   at SingleTest.singleVersionedNegTest@451-41.Invoke(String arg60) in D:\a\1\s\tests\fsharp\single-test.fs:line 451
   at SingleTest.singleVersionedNegTest(TestConfig cfg, String version, String testname) in D:\a\1\s\tests\fsharp\single-test.fs:line 451
   at SingleTest.singleNegTest(TestConfig cfg, String testname) in D:\a\1\s\tests\fsharp\single-test.fs:line 453
   at FSharp.Tests.Core.TypecheckTests.type check neg16() in D:\a\1\s\tests\fsharp\tests.fs:line 2399
  Standard Output Messages:
 ------------------ D:\a\1\s\tests\fsharp\typecheck\sigs ---------------

…-pattern

dsyme · 2020-07-15T10:11:38Z

@KevinRansom @cartermp this is ready

cartermp

This is missing test cases for patterns - at a minimum, the example from fsharp/fslang-suggestions#841 should be used

dsyme · 2020-07-17T10:45:17Z

This is missing test cases for patterns - at a minimum, the example from fsharp/fslang-suggestions#841 should be used

thanks, will add those now

…-pattern

abelbraaksma · 2020-07-23T21:00:17Z

From the original post by @Tarmil here I noticed two unchecked items that I didn't see in the commits, are these tracked/added elsewhere or already part of this PR? Namely, syntax coloring and code completion.

cartermp · 2020-07-27T18:34:08Z

https://youtu.be/AJQ3TM-p2QI

cartermp · 2020-07-27T20:03:04Z

Same failure - I feel like it should either be deleted or moved to non-FSharpQA since there's no diagnostics on why it failed

dsyme · 2020-07-27T22:51:04Z

@cartermp I agree

Tbh I've confirmed the case by hand, I think we can deal with this separately.

So as far as I'm concerned this is now ready

cartermp

I can see where #7416 is resolved in the code. Is there a way to verify #8661 in tests at all? I guess it's tricky since everything has the same name in source, so it's a name resolution test and not an integration test. I don't quite understand name resolution enough to see how that gets resolved. But the test coverage here is adequate for the scenario so I'd like to get this in for people to use in the next round of previews.

cartermp · 2020-07-28T15:03:39Z

src/fsharp/TypeChecker.fs

+            let resolvedToModuleOrNamespaceName =
+                if delayed.IsEmpty then
+                    let id,rest = List.headAndTail longId
+                    match ResolveLongIndentAsModuleOrNamespaceOrStaticClass cenv.tcSink ResultCollectionSettings.AllResults cenv.amap m false true OpenQualified env.eNameResEnv ad id rest true with 


Note that with open type declarations coming in this call may no longer do what is assumed, see here: https://github.com/dotnet/fsharp/pull/9513/files#diff-dbb9e59b8ac1e1e04e1b00282a91ec97R2192

Does that have any impact? I would expect not...

TIHan · 2020-07-29T18:16:01Z

I'm going to be looking at this today.

TIHan

Instead of having nameof be a special case in name resolution, couldn't we just add an Obsolete attribute with this message?:

[<Obsolete("'nameof' is not supported in this version of your compiler.")>]
let nameof<'T> = ..

This will prevent older compilers from calling it.
Newer compilers will ignore the Obsolete attribute. When it detects it's using an older language version, just throw an error saying it's not supported in this version, use tryLanguageFeatureErrorRecover from ErrorLogger to perform the check and raise the error.

It feels a bit icky to have nameof be a special case in name resolution. Ultimately, we just want an error at the end of the day.

KevinRansom · 2020-07-29T23:00:34Z

Instead of having nameof be a special case in name resolution, couldn't we just add an Obsolete attribute with this message?:

nameof has existed in FSharp.Core for over a year, so deprecating it will not reliably work. :-(

KevinRansom

Thanks for this work.

Kevin

TIHan · 2020-07-30T19:49:01Z

nameof has existed in FSharp.Core for over a year, so deprecating it will not reliably work. :-(

nameof is preview and its implementation throws an exception as it's treated specially by the compiler. It's fine that we put an Obsolete attribute on it.

dsyme · 2020-07-31T16:18:08Z

@KevinRansom shouldn't nameof have this?

        [<Experimental(ExperimentalAttributeMessages.RequiresPreview)>]

cartermp · 2020-07-31T16:22:47Z

The experimental attribute is only while things are in preview. Since we are moving nameof to F# 5 (which is still in preview, so no guarantees about current behavior as-shipped yet), that attribute was removed. We're also removing it for most other things now that we're nearing the end of the development cycle.

cartermp · 2020-07-31T16:24:22Z

The current experience today is that you can still call nameof in non-F# 5 code, but it's a compile error. So if Obsolete makes that a bit clearer, that's great. If it filters nameof out of a completion list for older F# users, then that's also great.

dsyme · 2020-07-31T16:38:06Z

I am trialling this, it should do the trick for giving a reasonable message to old compilers a25541a

dsyme · 2020-07-31T16:39:26Z

I am trialling this, it should do the trick for giving a reasonable message to old compilers a25541a

If this doesn't give the result we need then we can forget it I think

dsyme · 2020-07-31T20:47:27Z

THis is ready to go

KevinRansom · 2020-07-31T23:04:12Z

@dsyme , attributes on nameof are not going to be particularly helpful, because the method has already shipped in fsharp.core.dll since I think 4.7.0. I suppose it would help, when trying to build an updated fsharp.core, however, it would be a breaking change due to the fact it's already shipped. Even though a successful build against an old library is merely a runtime error.

cartermp · 2020-07-31T23:09:26Z

I'm fine with the outcome of this. nameof was only ever supported as preview and subject to change. Anyone who is bitten by this will simply have to make a change due to the preview nature of the feature.

I think in practice people aren't going to really be hit by this though.

dsyme · 2020-08-01T00:46:46Z

Yes, I think adding the attribute is best regarded as a good bug fix.

KevinRansom · 2020-08-01T00:48:35Z

Certainly there is no harm doing so.

dsyme · 2020-08-01T03:10:14Z

Let's merge this baby and enable this in 5.0

TIHan

Changes look good. Thank you @Tarmil for kick-starting the enhancements.

I think this is now ready.

* 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>

Tarmil added 2 commits March 5, 2020 23:17

Implement nameof x as a constant pattern

012d84b

Resolve ident to find nameof in patterns

4a861b9

merge master

c368b7b

dsyme added 3 commits June 19, 2020 15:57

fix build

94dfe30

fix build

162d02f

re-enable tests

ef706ee

cartermp reviewed Jun 19, 2020

View reviewed changes

tests/fsharp/core/nameof/preview/test.fsx Outdated Show resolved Hide resolved

dsyme added 2 commits June 19, 2020 16:30

fix test

d58cae0

fix operators

e7e31f9

cartermp changed the title ~~[WIP] Allow using nameof in pattern matching~~ Further enhancements to nameof Jun 19, 2020

cartermp reviewed Jun 19, 2020

View reviewed changes

tests/fsharp/core/nameof/preview/test.fsx Show resolved Hide resolved

cartermp assigned dsyme Jun 27, 2020

cartermp added the Candidate for F# 5 label Jun 27, 2020

dsyme force-pushed the nameof-pattern branch from 919042c to e7e31f9 Compare July 14, 2020 19:53

dsyme added 2 commits July 14, 2020 20:53

Merge branch 'master' of https://github.com/dotnet/fsharp into nameof…

982f5ad

…-pattern

align code

a3c298d

jaredpar mentioned this pull request Jul 14, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures dotnet/runtime#34472

Closed

cartermp suggested changes Jul 15, 2020

View reviewed changes

Merge branch 'master' of https://github.com/dotnet/fsharp into nameof…

82aaef0

…-pattern

cartermp closed this Jul 23, 2020

cartermp reopened this Jul 23, 2020

'fix test'

3be989a

'fix test'

95a328f

cartermp approved these changes Jul 28, 2020

View reviewed changes

TIHan reviewed Jul 29, 2020

View reviewed changes

KevinRansom approved these changes Jul 29, 2020

View reviewed changes

add message for C# and old compilers

a25541a

merge

25d1c78

dsyme added 2 commits July 31, 2020 17:41

fix build

ab31fe5

suppress error 3501 when a compiler supports nameof

21da062

TIHan approved these changes Aug 4, 2020

View reviewed changes

TIHan merged commit ae5c1db into dotnet:master Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further enhancements to nameof #8754

Further enhancements to nameof #8754

Tarmil commented Mar 19, 2020 •

edited by dsyme

Loading

dnfclas commented Mar 19, 2020 •

edited

Loading

CartBlanche commented Jun 1, 2020

Tarmil commented Jun 2, 2020

dsyme commented Jun 19, 2020

dsyme commented Jun 19, 2020

KevinRansom commented Jun 22, 2020

dsyme commented Jul 15, 2020

cartermp left a comment

dsyme commented Jul 17, 2020

abelbraaksma commented Jul 23, 2020

cartermp commented Jul 27, 2020

cartermp commented Jul 27, 2020

dsyme commented Jul 27, 2020

cartermp left a comment

cartermp Jul 28, 2020

TIHan commented Jul 29, 2020

TIHan left a comment •

edited

Loading

KevinRansom commented Jul 29, 2020

KevinRansom left a comment

TIHan commented Jul 30, 2020

dsyme commented Jul 31, 2020

cartermp commented Jul 31, 2020

cartermp commented Jul 31, 2020

dsyme commented Jul 31, 2020

dsyme commented Jul 31, 2020

dsyme commented Jul 31, 2020

KevinRansom commented Jul 31, 2020 •

edited

Loading

cartermp commented Jul 31, 2020

dsyme commented Aug 1, 2020 •

edited

Loading

KevinRansom commented Aug 1, 2020

dsyme commented Aug 1, 2020

TIHan left a comment

Further enhancements to nameof #8754

Further enhancements to nameof #8754

Conversation

Tarmil commented Mar 19, 2020 • edited by dsyme Loading

dnfclas commented Mar 19, 2020 • edited Loading

CartBlanche commented Jun 1, 2020

Tarmil commented Jun 2, 2020

dsyme commented Jun 19, 2020

dsyme commented Jun 19, 2020

KevinRansom commented Jun 22, 2020

dsyme commented Jul 15, 2020

cartermp left a comment

Choose a reason for hiding this comment

dsyme commented Jul 17, 2020

abelbraaksma commented Jul 23, 2020

cartermp commented Jul 27, 2020

cartermp commented Jul 27, 2020

dsyme commented Jul 27, 2020

cartermp left a comment

Choose a reason for hiding this comment

cartermp Jul 28, 2020

Choose a reason for hiding this comment

TIHan commented Jul 29, 2020

TIHan left a comment • edited Loading

Choose a reason for hiding this comment

KevinRansom commented Jul 29, 2020

KevinRansom left a comment

Choose a reason for hiding this comment

TIHan commented Jul 30, 2020

dsyme commented Jul 31, 2020

cartermp commented Jul 31, 2020

cartermp commented Jul 31, 2020

dsyme commented Jul 31, 2020

dsyme commented Jul 31, 2020

dsyme commented Jul 31, 2020

KevinRansom commented Jul 31, 2020 • edited Loading

cartermp commented Jul 31, 2020

dsyme commented Aug 1, 2020 • edited Loading

KevinRansom commented Aug 1, 2020

dsyme commented Aug 1, 2020

TIHan left a comment

Choose a reason for hiding this comment

Tarmil commented Mar 19, 2020 •

edited by dsyme

Loading

dnfclas commented Mar 19, 2020 •

edited

Loading

TIHan left a comment •

edited

Loading

KevinRansom commented Jul 31, 2020 •

edited

Loading

dsyme commented Aug 1, 2020 •

edited

Loading