Generate record stubs #365

OkayX6 · 2014-04-21T02:42:12Z

First POC

Status:

Works for basic scenarios
Handle situation when LBrace is on a different line
Only generate missing fields
Do not display Smart Tag when all fields are filled out
Support let x = { Field1 = } expression
Support let x = { MyRecord.Field1 = } expression

See:

vasily-kirichenko · 2014-04-21T04:24:06Z

Can you please squash all these merge commits?

vasily-kirichenko · 2014-04-21T04:28:31Z

src/FSharpVSPowerTools/FSharpVSPowerTools.csproj

Do not turn deployment in the Release configuration. It breaks our CI build.

dungpa · 2014-04-21T05:36:40Z

First suggestion: don't use failwith on record fields; it's going to fail when the code is executed.

Please use known default values for primitive types (those in this table http://stackoverflow.com/questions/17251025/what-where-is-get-zero-in-fs-int/17251353#17251353 plus string type). For other types, use Unchecked.defaultof<_>. So you should keep DefaultFieldValue as a function from type to string and pass this information into the context.

vasily-kirichenko · 2014-04-21T10:31:54Z

src/FSharpVSPowerTools.Core/RecordStubGenerator.fs

I don't think this convention is very good. It's unlikely you'll use any of this ignored values in the future. The following code reads much easier:

| SynMemberDefn.AutoProperty(_, _, _, _, _, _, _, _, expr, _, _) ->

Copy paste driven development from Anh-Dung :)
I'll clean it up

In general I agree.

However, when dealing with ASTs, I prefer to keep available information since it gives the structural view of the ASTs. When you do bug fixing or look at the code after a few months, it's really hard to know what you did miss. We shouldn't rely on intellisense to know about the structural info.

I agree with @dungpa, dealing with large number of positional wildcards can be difficult - in fact it was one of motivations behind adding named fields for DUs so matching can be performed selectively by name

@dungpa Why do we not use 3.1 compiler? Does the CI server not support it? Or VS 2012? I remember I added names to https://github.com/fsprojects/VisualFSharpPowerTools/blob/master/src/FSharpVSPowerTools.Core/XmlDocParser.fs#L4 and the solution compiled OK.

We still keep a VS2012 solution around and use it to diagnose problems in VS2012. For that purpose, the source code only uses the subset supported by F# 3.0. I'm not sure how to deal with versioning issues properly.

I thought Visual F# Tools 3.1.1 update supports VS 2012.

OkayX6 · 2014-04-21T14:34:38Z

@dungpa Isn't that what we want? I did it on purpose, because if you put default values you can easily forget to initialize some fields.

To you, what's the difference with interface implementation stubs that raise exceptions?

What's your opinion @vasily-kirichenko?

OkayX6 · 2014-04-21T14:56:42Z

How do you squash these merge commits when they're older than the moment where the branch was created? Moreover they're interleaved with other commits?

vasily-kirichenko · 2014-04-21T18:35:43Z

@OkayX6 Something like this:

let r: R = { F1 = <string>; F2 = <DU1>; F3 = <Choice<int option, exn>>; F4 = { FieldOfAnotherRecord1 = <int> }}

It's not compilable and has nice type hints.

vasily-kirichenko · 2014-04-21T18:42:56Z

That's all nice, but I cannot remember I've ever written

let r: R = { ...

What about let f { F1 = f1 } = ...? Or match x with { F1 = f1 } -> ...? Or let x = { F1 = { ...? You simply don't have a place to put the smartag. This is why I believe the @7sharp9 's "intellisence like for overloaded methods" would work very, very naturally.

dungpa · 2014-04-21T20:15:42Z

We don't do intellisense so autocompletion is probably out of the question.

Could we do something like:

When users type let x = { F1, show smart tags on F1
All possible records with F1 fields are shown on the list of smart tags (a single smart tag provider could provide multiple smart tags)
When users choose one from the list, complete the source with remaining fields.

It effectively works as intellisense, instead of pressing Ctrl + Space, we use mouse and click on contextual smart tags.

What do you think?

ovatsus · 2014-04-21T20:17:56Z

Also consider the name of the record type itself on that list, as you can do let x = { R1.F1 = ... }

7sharp9 · 2014-04-21T21:33:38Z

The simplest thing I can think of is is to inject the currently defined record types (in the source/ project/ reference) into the completion list mechanism. I think that's how I would approach it in XS. Ive not been keeping up with the api changes in FCS is there an efficient typed symbol search mechanism?

On 21 Apr 2014, at 19:42, Vasily Kirichenko notifications@github.com wrote:

This all nice, but I cannot remember I've ever written

let r: R = { ...
What about let f { F1 = f1 } = ...? Or match x with { F1 = f1 } -> ...? Or let x = { F1 = { ...? You simply don't have a place to put the smartag. This is why I believe the @7sharp9 's "intellisence like for overloaded methods" would work very, very naturally.

—
Reply to this email directly or view it on GitHub.

OkayX6 · 2014-04-21T23:00:36Z

@vasily-kirichenko Ok, I like your suggestion.

@dungpa @ovatsus Yes these are nice options too. We can do all of them, as we don't always remember record field names exactly.

@7sharp9 That's what I wanted to do as it appeared to be the best approach to me. But I didn't know how to play with the intellisense mechanism.
In FCS, you can use GetAllUsesOfAllSymbols, so you can easily retrieve all accessible record types' metadata.

Do you think http://msdn.microsoft.com/en-us/library/vstudio/ee372314(v=vs.100).aspx is the way to go?

How does it interact with the existing intellisense? Does it add completion or does it replace completion?
Mimicking the implement interface src code was the easiest thing to do for me as I didn't have time to investigate the completion thing.

If someone is willing to investigate the completion list mechanism, please do. I might still continue experimenting, because I think we'd still be able to reuse most of the code.

vasily-kirichenko · 2014-04-22T04:24:45Z

@dungpa I like your idea about showing the smarttag when user enters a field (or maybe part / start of a field? or even *?) Is it possible to show the smarttag on a space, like at the end of let x = {?

Yes, it's quite a poorer solution compared to proper intellisence, but touching the former is too risky and can end up with endless fighting with Visual F# Tools intellisense.

@OkayX6 However, if you feel lucky and full of energy to investigate the intellisence approach, it would bring us a really high quality solution.

dungpa · 2014-04-22T04:33:59Z

@OkayX6 Yes, it's the right interface for intellisense. It most likely adds another tab to intellisense windows (see http://stackoverflow.com/questions/10460138/custom-intellisense-extension) but we should try to inject current intellisense tab. WebEssentials has many examples of custom intellisense so I don't think it's an issue https://github.com/madskristensen/WebEssentials2013/search?q=ICompletionSource&ref=cmdform

@vasily-kirichenko AFAIK we can capture key press and display smart tag at any location.

dungpa · 2014-04-22T04:40:28Z

I still think stub generation should give compilable and usable values. Something like:

let r: R = { F1 = Unchecked.defaultof<string>; F2 = Unchecked.defaultof<DU1>; 
             F3 = Unchecked.defaultof<Choice<int option, exn>>}

When users use this feature, I think they know how to enter appropriate values to customize stub values. The use Unchecked.defaultof signals that it's a generated place holder. In many cases, users need quick generation of values for testing. Forcing them to fix uncompilable or fail-at-runtime generated code is not nice.

@ovatsus @7sharp9 Any suggestions?

vasily-kirichenko · 2014-04-22T04:42:29Z

If we master intellisence, we could also fix the places where VFT does not show any (property initializers in class constructors and curried function arguments for instance). It would be cool.

vasily-kirichenko · 2014-04-22T04:45:33Z

@dungpa I see your point. But it's also dangerous since it's too easy for the user to forget to initialize some fields with proper values.

dsyme · 2014-04-22T09:08:37Z

How about the text "nyi"?

dungpa · 2014-04-22T09:44:39Z

Ok, let's keep the current behaviour.

I think we should implement options for code generation similar to those of ReSharper http://www.jetbrains.com/resharper/webhelp/Reference__Options__Languages__Common__Generated_Members.html so that personal preferences can be taken into account.

OkayX6 · 2014-04-22T22:29:26Z

Just to investigate what kind of data the intellisense component would be facing, I've looked at the AST when you're writing:

let x = { }
let x = { Field1 }
let x = { MyRecord.Field1 }
let x = { Field1 = 0 }

and the expression on the right-hand of the binding is considered respectively:

a record
a computation expression
a computation expression
a record (that means you have to go pretty far to be entitled to use the code generation helper)

Technically it's not an issue but it'd be a bit hacky if we treat them as special cases.
I don't know if it's possible or worth changing the parser behavior so that such expressions would be considered as records in priority?

OkayX6 · 2014-04-22T23:41:24Z

@ovatsus Your suggestion is more or less implemented :) (see the second GIF). I had to specify a dummy value to the field for the parser to identify it as a record expression (and with the field symbol included in the tree).

ovatsus · 2014-04-23T01:09:43Z

Cool :)

vasily-kirichenko · 2014-04-23T04:32:52Z

@OkayX6 It definitely looks more useful now.

dungpa · 2014-04-23T06:45:24Z

@OkayX6 I don't think we have to alter behaviours of the parser. We could accept the fact that the ASTs could return computation expressions and look for symbols based on the identifiers. If the symbol is an FSharpField of an record type, or an FSharpSymbol with IsFSharpRecord = true then smart tags should be displayed. In that way, we don't need to specify dummy values.

It's difficult to expect the parser to return what we want when the information is incomplete. In general, we should always do filtering based on multiple sources (ASTs, symbols, and in some cases lexer tokens).

OkayX6 · 2014-04-23T09:23:22Z

For that, we need GetSymbolUseAtLocation to be precise enough.

Investigation results depending on caret position & source code:

let x = { Field1 }
- Caret position: Field1 -> no symbol use
let x = { Field1 = }
- Caret position: Field1 -> no symbol use
let x = { MyRecord.Field1 } ==> That's the best we can do as of today
- Caret position: MyRecord -> FSharpSymbol found with IsFSharpRecord = true
- Caret position: Field1 -> no symbol use
let x = { MyRecord.Field1 = }
- Caret position: MyRecord -> FSharpSymbol found with IsFSharpRecord = true
- Caret position: Field1 -> no symbol use

OkayX6 · 2014-04-23T09:28:03Z

I'm OK with accepting computation expressions, I'm just concerned we take too many dependencies on accidental behaviors of the parser. It would make it easy to break the component if the parser changes its behavior (if for instance, they enhance it so that it can parse more degraded source code).

But maybe I'm too pessimistic?

dungpa · 2014-04-23T21:32:33Z

I don't think the parser is that easy to change. Visual F# Tools has strong dependencies on it; it's difficult to revise the parser without breaking changes there.

Regarding intellisense - assume that when users type let x = { and invoke autocompletion, call GetAllUsesOfAllSymbols, keeps all records and inject generated code to autocompletion dialog.
In general it could slow down intellisense dramatically because we need to type check and generate multiple records when autocompletion is invoked. The benefit is that UI workflow seems to be much easier with intellisense.

OkayX6 · 2014-04-23T23:13:05Z

Fair enough.
I'll be out of town for the rest of the week, if you want to have a look at the current work (I've completed one more task)...

I think that you'll be able to reuse most of the code that deals with the AST.

Regarding intellisense though, you'll need GetAllUsesOfAllSymbols to be exposed by FCS as currently, only GetAllUsesOfAllSymbolsInFile is exposed (file-wide, not solution-wide).

OkayX6 · 2014-04-27T22:33:11Z

I managed to clean up the history.

dungpa · 2014-04-28T22:52:17Z

I think the merge commits appear again now.

dungpa · 2014-05-09T15:17:17Z

How's your intellisense experiment going?

If you finish the last TODO items and rebase to master, we should be able to merge this.

OkayX6 · 2014-05-10T00:12:36Z

Ha, in fact I thought that you or Vasily wanted to take over the work I had done, and make it use the Intellisense UI instead of Smart Tags.

This is why I kind of stopped working on it.

I acknowledge your comment so I can finish the last TODO item by the beginning of next week (I won't have access to my laptop before).

(As per using the Intellisense UI, I didn't go far, I just managed to get the MSDN walkthrough working. The API behaved a bit weird to me when I tried to augment the existing completion list instead of creating a new Tab)

// Minimum let x: MyRecord = { }

When the record expression didn't start at the same line as the type annotation, then the Smart Tagger wouldn't know where to insert the stubs.

OkayX6 · 2014-05-16T16:14:01Z

I've rebased my branch and resolved the latest TODO item.

7sharp9 · 2014-05-16T17:00:59Z

It would be nice to share the code for the F# addin in XS, we've already borrowed the implement interface stuff because theres no sharing mechanism for us to use yet...

vasily-kirichenko · 2014-05-16T17:15:51Z

So, it seems to work in two scenarios:

let _ = { Reco|rd.Field = "valid value" }

and

let _: Record = { Fi|eld = "valid value" }

Am I right?

vasily-kirichenko · 2014-05-16T17:17:33Z

@7sharp9 We're going to use this as a temporary solution only, until number of FCS bugs are fixed. So, I don't think you should use it in Xamarin.

7sharp9 · 2014-05-16T17:19:23Z

@vasily-kirichenko I wouldn't put it in until it was stable anyway, implement interface is in the latest master though.

OkayX6 · 2014-05-16T18:00:43Z

Three scenarios: let x = { Field1 = 0 } is the latest addition.

vasily-kirichenko · 2014-05-16T18:08:23Z

@OkayX6 Oh, this last scenario is great!

Generate record stubs

vasily-kirichenko reviewed Apr 21, 2014
View reviewed changes

OkayX6 mentioned this pull request May 14, 2014

Get types available at current scope fsharp/fsharp-compiler-docs#144

Closed

OkayX6 added 17 commits May 16, 2014 14:25

Record stub test skeleton

093992a

First try of record expression locator

c25dc03

Locate Record expression binding

d8594d9

// Minimum let x: MyRecord = { }

Get symbol use at caret location

10a1327

Record stub generation capability

c806105

Enhance unit tests

b8732c8

Add failing test

880ece3

Factor ColumnIndendedTextWriter out

1a25042

General cleanup

626f536

Revert VSIX deployment in Release mode

37dabc5

Fix issue with LBrace position

17f8fe6

When the record expression didn't start at the same line as the type annotation, then the Smart Tagger wouldn't know where to insert the stubs.

Generate record fields needed only

69e89fd

Support for another pattern

8392d50

SmartTag doesn't appear expr is complete

7767831

Adapt with VS Language Service interface change

b431037

Cleanup

83e970f

Support non-qualified field expression

a1f5c8c

vasily-kirichenko added a commit that referenced this pull request May 17, 2014

Merge pull request #365 from OkayX6/generate-record-stubs

3e53035

Generate record stubs

vasily-kirichenko merged commit 3e53035 into fsprojects-archive:master May 17, 2014

OkayX6 deleted the generate-record-stubs branch May 19, 2014 23:30

Generate record stubs #365

Generate record stubs #365

Uh oh!

Conversation

OkayX6 commented Apr 21, 2014

Uh oh!

vasily-kirichenko commented Apr 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dungpa commented Apr 21, 2014

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!

OkayX6 commented Apr 21, 2014

Uh oh!

OkayX6 commented Apr 21, 2014

Uh oh!

vasily-kirichenko commented Apr 21, 2014

Uh oh!

vasily-kirichenko commented Apr 21, 2014

Uh oh!

dungpa commented Apr 21, 2014

Uh oh!

ovatsus commented Apr 21, 2014

Uh oh!

7sharp9 commented Apr 21, 2014

Uh oh!

OkayX6 commented Apr 21, 2014

Uh oh!

vasily-kirichenko commented Apr 22, 2014

Uh oh!

dungpa commented Apr 22, 2014

Uh oh!

dungpa commented Apr 22, 2014

Uh oh!

vasily-kirichenko commented Apr 22, 2014

Uh oh!

vasily-kirichenko commented Apr 22, 2014

Uh oh!

dsyme commented Apr 22, 2014

Uh oh!

dungpa commented Apr 22, 2014

Uh oh!

OkayX6 commented Apr 22, 2014

Uh oh!

OkayX6 commented Apr 22, 2014

Uh oh!

ovatsus commented Apr 23, 2014

Uh oh!

vasily-kirichenko commented Apr 23, 2014

Uh oh!

dungpa commented Apr 23, 2014

Uh oh!

OkayX6 commented Apr 23, 2014

Uh oh!

OkayX6 commented Apr 23, 2014

Uh oh!

dungpa commented Apr 23, 2014

Uh oh!

OkayX6 commented Apr 23, 2014

Uh oh!

OkayX6 commented Apr 27, 2014

Uh oh!

dungpa commented Apr 28, 2014

Uh oh!

dungpa commented May 9, 2014

Uh oh!