Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Code Fixes in F# (RC3 regression) #16799

Merged
merged 4 commits into from
Jan 27, 2017

Conversation

saul
Copy link
Contributor

@saul saul commented Jan 27, 2017

Customer scenario

In RC3, Code Fixes no longer work in F#. The F# community has put in a significant amount of effort to add new Roslyn-based Code Fixes and it would be a great shame if we can't get this fixed for RTM.

Bugs this fixes:

Fixes dotnet/fsharp#2329

Workarounds, if any

No workaround

Risk

No risk

Performance impact

Low. Will act the same as RC2

Is this a regression from a previous update?

This is a regression from RC3

How was the bug found?

F# contributor testing

@dnfclas
Copy link

dnfclas commented Jan 27, 2017

Hi @saul, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented Jan 27, 2017

@saul, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@saul
Copy link
Contributor Author

saul commented Jan 27, 2017

Tagging @CyrusNajmabadi as this is a result of #15961

@CyrusNajmabadi
Copy link
Member

I think the better fix would be to just go back to:

[VisualStudio.Utilities.ContentType(ContentTypeNames.RoslynContentType)]

@cartermp Do we have F# code actions now? I disabled this a while back because of an F# bug that said this should be disabled due to it having no code actions.

@saul
Copy link
Contributor Author

saul commented Jan 27, 2017

I'm happy to do that too. If I make that change will this PR be good to go into RTM?

See @cartermp's comment on dotnet/fsharp#2329 for confirmation.

@CyrusNajmabadi
Copy link
Member

@cartermp Can you talk to @natidea and @MattGertz about if this meets the bar at this point? Thanks!

@CyrusNajmabadi
Copy link
Member

@saul Please make the change. I cannot guarantee anything unfortunately. However, my hope is that we would take this as it's very low risk, and clearly provides a lot of value for the F# user base.

@MattGertz
Copy link
Contributor

Can I get clarification on how this regressed? As Cyrus says, the bar is extremely high at this point -- basically major perf issues and crashes. I might be able to sell a regression, but I'd need to know a lot more about what happened. Thanks.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 27, 2017

@MattGertz The timeline:

  1. Roslyn originally exported the Code-Action (lightbulbs) infrastructure for all Roslyn languages (i.e. C#/VB/TS/JS/F#).
  2. Around 5-6 weeks ago, at the request of F#, Roslyn disabled several features for F# due to them not being supported by that language (yet). For example, we made it so that F# could disable 'go to implementation' so it would not show up in their completion list. We were also asked to disable 'code action' support.
  3. It now turns out that F# was working on Code-Actions, and did want them to ship. They just were not ready at that point in time.

So, prior to the change made in December (#15961), F# would have been able to finish up their code-action work, and ship it, and it would just light up. Now, it won't even light up, even though they've done their side.

The fix is very simple, and is essentially just returning us to the state we were at prior to my change.

--

Note: I cannot determine if this should be classified as a 'regression' or not. In one sense, it's not. F# code actions have never shipped. So there is no user visible diminishing of features. However, in another sense it is. The product was previously setup so that F# could ship their Code-Action features if they felt they were ready. Then, we changed it so that even if they got ready, they could not ship it. So, we regressed the 'ability' for them to ship this stuff on their own.

@MattGertz
Copy link
Contributor

Thanks, @CyrusNajmabadi , that helps a lot, and reverting to previous state is also a happy solution in Shiproom -- less risky. Any chance this is ready for today's Shiproom? This would be much easier to take today as well -- bar next week will be into nosebleed territory.

@CyrusNajmabadi
Copy link
Member

👍

@CyrusNajmabadi
Copy link
Member

let me get another signoff @MattGertz

@MattGertz
Copy link
Contributor

Great. Gimme a VSO bug w/template copied & set to Submit, and we're all set to go, then.

@CyrusNajmabadi
Copy link
Member

@MattGertz Making VSO bug now.

@saul
Copy link
Contributor Author

saul commented Jan 27, 2017

Thanks for taking a look at this so quickly @CyrusNajmabadi and @MattGertz. As requested I've reverted to the state prior to Cyrus's #15961 PR.

@CyrusNajmabadi
Copy link
Member

@MattGertz
Copy link
Contributor

Perfect, & good timing. Leaving for Shiproom in a few minutes.

@MattGertz
Copy link
Contributor

Approved! :-)

@CyrusNajmabadi CyrusNajmabadi merged commit 69f1774 into dotnet:master Jan 27, 2017
@CyrusNajmabadi
Copy link
Member

Thanks!

@saul
Copy link
Contributor Author

saul commented Jan 27, 2017

Thanks for the quick turnaround - the rest of the F# community will really appreciate it :)

@CyrusNajmabadi
Copy link
Member

Thanks for the contribution! Note: we're basically almost completely shut down. But we'll still be totally happy to accept more stuff, just off of the post-dev15 branch. Thanks!

@cartermp
Copy link
Contributor

Big thanks @saul!

@isaacabraham
Copy link

Great that this is going back in - thanks all for doing it. To be fair, it seems the original request seems to have been to provide an option to disable the code fixes, not to actually disable it permanently, so there seems to have been a misunderstanding at some point - but it's a relief that this has been undone. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RC3] Code Fixes do not work
7 participants