[WIP] [FS-1003] Nameof operator reimplementation #2290

vasily-kirichenko · 2017-01-18T13:56:28Z

Implementation of RFC FS-1003

…iteral>)`

Conflicts: src/fsharp/FSComp.txt src/fsharp/FSharp.Core.Unittests/SurfaceArea.Silverlight.2.0.fs src/fsharp/FSharp.Core.Unittests/SurfaceArea.net20.fs src/fsharp/PostInferenceChecks.fs src/fsharp/TastOps.fs src/fsharp/TcGlobals.fs

This reverts commit 529cc6f.

This reverts commit 223d313. Conflicts: tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfAdditionExpr.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfAppliedFunction.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfAsAFunction.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfDictLookup.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfIntConst.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfIntegerAppliedFunction.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfParameterAppliedFunction.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfPartiallyAppliedFunction.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfStringConst.fs tests/fsharpqa/Source/Conformance/Expressions/DataExpressions/NameOf/E_NameOfWithPipe.fs

forki · 2017-01-18T14:39:32Z

This looks promising. Please keep us posted about progress.

vladima · 2017-01-18T18:04:13Z

src/fsharp/TypeChecker.fs

+        | SynExpr.LongIdent(_, LongIdentWithDots(idents, _), _, _) when not (isNil idents) ->
+            match expr with
+            // `nameof` operator
+            | ApplicableExpr (_, Expr.App(Expr.Const(Const.String("nameof"), _, _), _, _, _, _), _) -> // no idea really what shape we should match on here, will check in debugger


this is technically a breaking change when user-defined function nameof exists in the scope. I think it should resolve the name and only treat it as nameof operator if it it was resolved to intrinsic from FSharp.Core

@vladima thanks! Any point how to do it? :)

@vladima I think I've found out the solution.

vasily-kirichenko · 2017-01-18T20:42:32Z

It works:

vasily-kirichenko · 2017-01-18T20:43:50Z

I think we should color it as keyword
I wont work at attribute application site (I guess)
Do we need typenameof as well?

forki · 2017-01-18T21:00:32Z

❤️

saul · 2017-01-18T23:25:33Z

This is really cool :)

forki · 2017-01-19T07:25:15Z

Yeah looks much better than my original implementation. Next step would be tests for doing this in attributes.

it raises a proper error if applicated to non (Long)Ident arg

vasily-kirichenko · 2017-01-19T07:55:52Z

@forki looks like it already works in attributes:

I haven't checked the generated IL though.

smoothdeveloper · 2017-01-19T08:08:24Z

@vasily-kirichenko, what is the value of nameof foo in this session? is nameof returning the compiled name or the F# name when [<CompiledName>] is used?

vasily-kirichenko · 2017-01-19T08:09:37Z

@smoothdeveloper no, it returns "foo". I think that's how it's supposed to work.

smoothdeveloper · 2017-01-19T08:10:28Z

That will give some surprise for those doing reflection, but yes I agree it is expected behaviour. We will need compilednameof in the future 😃

vasily-kirichenko · 2017-01-19T09:56:55Z

The problem: currently we can use nameof as normal function:

call it with pipes System.DateTime |> nameof
pass it to other functions let x = foo nameof

So we should restrict places where nameof can appear.

vasily-kirichenko · 2017-01-19T10:19:26Z

The above's been fixed.

vasily-kirichenko · 2017-01-19T10:49:52Z

All cases from the list at the bottom of the RFC https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1003-nameof-operator.md work, except the following two:

works with provided symbol names, i.e. generated by type providers - not checked yet
can be used in pattern matching - does not work:

works with type arguments, let f<'t> (x : 't) = nameof 't

I don't think we should support the last case at all.

@dsyme any points where to fix the case with pattern matching? I think it's quite important.

dsyme · 2017-01-19T12:53:59Z

(I'm going to hold off reviewing this and the RFC until VS20117 RTM is done and dusted if that's ok)

abelbraaksma · 2017-09-23T14:09:03Z

@vasily-kirichenko, well, it would have been an easy resolution for what I recently encountered with running certain tests, see #3632, but I'd understand, one swallow doesn't make it Spring (Dutch proverb ;)).

ReedCopsey · 2017-09-23T18:41:08Z

The fundamental problem with this feature: nobody wants it to be implemented. We could make another round of discussion, but what’s the point?

@vasily-kirichenko That's just not true - many users (including myself) would love for this to be implemented. It's very useful in certain types of applications.

realvictorprm · 2017-09-23T22:47:31Z

I consider this to be fundamental. Therefor I think there isn't any argument against implementing it.
I'm going to schedule this within the next week's.

abelbraaksma · 2017-09-24T00:04:57Z

@realvictorprm, I think that this PR was close to being finished by @vasily-kirichenko. I'd be interested to understand the current limitations better and why they aren't resolvable (could someone elaborate with an example perhaps?), or whether we can live with those limitations (F# has more limitations when it comes to full support of whatever C# can do).

Sometimes covering 99% of all common cases is good enough, even more so if the other 1% can be worked around. Besides, it would be a waste to see all this work gone for naught.

forki · 2017-09-24T07:46:06Z

Look @vasily-kirichenko and myself tried to get it in (#13 and #908).

It looks like super trivial feature but it has many many edge cases. What it needs is someone who goes through all the cases and makes a list which shows what is implemented and what not. Then @dsyme can decide if it is enough

forki · 2017-09-24T07:47:59Z

(first version is over two years old)

dsyme · 2017-09-25T08:52:47Z

The fundamental problem with this feature: nobody wants it to be implemented.

This is not true

What it needs is someone who goes through all the cases and makes a list which shows what is implemented and what not. Then @dsyme can decide if it is enough

This is true

KevinRansom · 2018-09-12T18:26:49Z

@dsyme, @cartermp, @vasily-kirichenko,

I'm trying to cut down the number of inactive PR's on our repo. This seems to be quite dead. I think the best thing is to close it, and let someone emerge who wants this feature drive it to completion.

It has hung around without a concerted efforted for about 3 years now.

What do you think?

Kevin

vasily-kirichenko · 2018-09-12T18:52:41Z

I think the best thing is to close it, and let someone emerge who wants this feature drive it to completion.

So, Microsoft does not want to drive it? Why it wanted to drive the Span feature, but does not want to do this one? Who decides?

KevinRansom · 2018-09-12T18:56:46Z

@vasily-kirichenko

the span feature priority is irrelevant to this discussion.

vasily-kirichenko · 2018-09-12T18:57:55Z

I could close this PR, as I did before, but you guys love to reopen it, so feel free to close it yourself.

KevinRansom · 2018-09-12T19:19:16Z

Okay I am closing this PR.

I have created a branch:
https://github.com/Microsoft/visualfsharp/tree/nameof-operator with the changes. In case anyone wants to revive this work.

xperiandri · 2019-02-18T15:33:44Z

Is this feature complete?

slang25 · 2019-02-18T16:03:51Z

@xperiandri No, it needs reviving, rebasing, and taking over the finish line.

There's a venn diagram of

People with time
People with the incentive (i.e. this is a priority for them)
People with the expertise

and it's a slim intersection I believe.

cartermp · 2019-02-18T16:58:58Z

From a technical standpoint, the remaining work here is to handle:

Additional considerations as defined in the RFC (either with everything implemented or with some cut for specific reasons)
A much more complete RFC, as the current one is not satisfactory in terms of documenting the feature

dsyme · 2019-03-12T14:33:40Z

Revamped implementation at #6325

@vasily-kirichenko @forki Thank you for your work on this RFC so far.

Vasily Kirichenko added 5 commits January 18, 2017 16:24

substitute 'nameof(<long ident>) with Conts(<long ident as string l…

d380a0e

…iteral>)`

Implementing basic nameof and typenameof operators

223d313

Conflicts: src/fsharp/FSComp.txt src/fsharp/FSharp.Core.Unittests/SurfaceArea.Silverlight.2.0.fs src/fsharp/FSharp.Core.Unittests/SurfaceArea.net20.fs src/fsharp/PostInferenceChecks.fs src/fsharp/TastOps.fs src/fsharp/TcGlobals.fs

Apply feedback

529cc6f

Revert "Apply feedback"

f3c4ba5

This reverts commit 529cc6f.

msftclas added the cla-already-signed label Jan 18, 2017

revert nameof and typenameof functions to FSharp.Core

77c8f77

vladima reviewed Jan 18, 2017

View reviewed changes

vasily-kirichenko added 2 commits January 18, 2017 22:28

wip

3070c1d

it works

b3e712d

Merge remote-tracking branch 'origin/master' into nameof-operator

6c6e3b5

Vasily Kirichenko added 2 commits January 19, 2017 10:32

refactoring

7832be5

make it work on simple Idents

6581790

it raises a proper error if applicated to non (Long)Ident arg

use proper error message

0f5165f

restrict using nameof operator as first class

22898ad

remove typenameof bits

e5033af

msftgits removed the cla-already-signed label Sep 26, 2017

dsyme changed the title ~~[ RFC FS-1003 ] Nameof operator reimplementation~~ [FS-1003] Nameof operator reimplementation Dec 1, 2017

KevinRansom changed the title ~~[FS-1003] Nameof operator reimplementation~~ [WIP] [FS-1003] Nameof operator reimplementation Dec 7, 2017

KevinRansom changed the base branch from master to dev15.6 December 7, 2017 19:18

KevinRansom changed the base branch from dev15.6 to master December 7, 2017 19:19

KevinRansom removed the Candidate for F# 4.3 label Dec 7, 2017

dsyme added the Candidate for F# 4.2 label Dec 8, 2017

cartermp added Candidate for F# vNext and removed Candidate for F# 4.5 labels May 19, 2018

KevinRansom closed this Sep 12, 2018

cartermp removed the Candidate for F# vNext label Nov 12, 2018

This comment has been minimized.

Sign in to view

dsyme mentioned this pull request Mar 12, 2019

[RFC FS-1003] Nameof operator redux #6325

Closed

23 tasks

dsyme mentioned this pull request May 22, 2019

[RFC FS-1003] Nameof operator redux #6809

Merged

22 tasks

[WIP] [FS-1003] Nameof operator reimplementation #2290

[WIP] [FS-1003] Nameof operator reimplementation #2290

Uh oh!

Conversation

vasily-kirichenko commented Jan 18, 2017 • edited by dsyme Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forki commented Jan 18, 2017

Uh oh!

vladima Jan 18, 2017

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko Jan 18, 2017

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko Jan 18, 2017

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Jan 18, 2017

Uh oh!

vasily-kirichenko commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forki commented Jan 18, 2017

Uh oh!

saul commented Jan 18, 2017

Uh oh!

forki commented Jan 19, 2017

Uh oh!

vasily-kirichenko commented Jan 19, 2017

Uh oh!

smoothdeveloper commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasily-kirichenko commented Jan 19, 2017

Uh oh!

smoothdeveloper commented Jan 19, 2017

Uh oh!

vasily-kirichenko commented Jan 19, 2017

Uh oh!

vasily-kirichenko commented Jan 19, 2017

Uh oh!

vasily-kirichenko commented Jan 19, 2017

Uh oh!

dsyme commented Jan 19, 2017

Uh oh!

abelbraaksma commented Sep 23, 2017

Uh oh!

ReedCopsey commented Sep 23, 2017

Uh oh!

realvictorprm commented Sep 23, 2017

Uh oh!

abelbraaksma commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forki commented Sep 24, 2017

Uh oh!

forki commented Sep 24, 2017

Uh oh!

dsyme commented Sep 25, 2017

Uh oh!

KevinRansom commented Sep 12, 2018

Uh oh!

vasily-kirichenko commented Sep 12, 2018

Uh oh!

KevinRansom commented Sep 12, 2018

Uh oh!

vasily-kirichenko commented Sep 12, 2018

Uh oh!

KevinRansom commented Sep 12, 2018

Uh oh!

xperiandri commented Feb 18, 2019

Uh oh!

slang25 commented Feb 18, 2019

Uh oh!

This comment has been minimized.

cartermp commented Feb 18, 2019

Uh oh!

dsyme commented Mar 12, 2019

Uh oh!

vasily-kirichenko commented Jan 18, 2017 •

edited by dsyme

Loading

vasily-kirichenko commented Jan 18, 2017 •

edited

Loading

smoothdeveloper commented Jan 19, 2017 •

edited

Loading

abelbraaksma commented Sep 24, 2017 •

edited

Loading