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

Code fixes that impact additional files of custom item types no longer work #2160

Open
jasonmalinowski opened this issue May 11, 2017 · 5 comments
Labels
Feature Request Request for a new feature, or new scenario to be handled. Parity-Legacy-API Missing or behavior differences in APIs from the legacy project system. Parity-Legacy-Feature Missing features from the legacy project system. Triage-Approved Reviewed and prioritized
Milestone

Comments

@jasonmalinowski
Copy link
Member

jasonmalinowski commented May 11, 2017

Repro Steps

  1. Open Roslyn.sln, and add a public API somewhere in the CodeAnalysis.csproj project that triggers the PublicAPI checker.
  2. Ctrl+.. Observe the suggestion from the code fix.
  3. Apply it.

Expected: it works
Actual: it does nothing

Debugging on the Roslyn side of things, we are telling the RDT to save the item, but it bails because there is no hierarchy there:

    if (((grfSave & RDTSAVEOPT_SaveAllButThis) == 0) && docCookie)
    {
        // cant save a doc anything w/o a hierarchy
        if (NULL == info.spHier)
            return NOERROR;

Roslyn, when it opened the invisible editor, passes null for RegisterInvisibleEditor, which is documented that in this case it'll use whatever project "claims" the file. But if you look into the solution explorer, you'll see that PublicAPI.*.txt files aren't listed, and whatever data source drives that decision is also saying the project doesn't own the file for the purposes of the invisible editor. If you edit the project to change the item type to AdditionalFiles instead of PublicAPI, they now appear and code actions apply just fine.

The old project system was (to a fault) aggressive in counting unknown item group types as something to be "included" in the project; it seems the new project system is changing that behavior. The additional files with the PublicAPI item type do work otherwise in the IDE, as we do tell the compiler to consume them and they're coming through with the design time build.

There is an interesting question of whether Roslyn passing null for the IVsProject property is "bad" (we might have one we can figure out), but it seems this is a regression in behavior at a few potential levels which might have other impacts -- any other callers are broken and the IsDocumentInAProject API is observing different behavior.

@davkean
Copy link
Member

davkean commented May 11, 2017

@jasonmalinowski Interesting. From a standpoint of both legacy and CPS project systems, items that come through design-time builds are not considered part of the project/tree.

What's relevant here though, is that the legacy project system will treat any (visible) item in the project file as part of the tree - whereas, CPS doesn't. Only those item types that have been registered to show up the tree, show up in the tree, hence the behavior you are seeing.

@lifengl @jviau What are you thoughts on this? This is going to be compat thing with the legacy project system, very similar to the AvailableItemName - where we'll want to add item types without a ItemType registeration.

@davkean davkean added Bug This is a functional issue in already written code. Parity-Legacy-API Missing or behavior differences in APIs from the legacy project system. Feature Request Request for a new feature, or new scenario to be handled. Parity-Legacy-Feature Missing features from the legacy project system. and removed Bug This is a functional issue in already written code. labels May 11, 2017
@davkean davkean added this to the 16.0 milestone May 11, 2017
@jasonmalinowski
Copy link
Member Author

@davkean Is there any workaround for now I can check in to say those should be counted as registered?

@davkean
Copy link
Member

davkean commented May 11, 2017

The workaround would be to mark them as AdditionalFiles or None or Content, or (harder) you could add your own registration, similar to:
https://github.com/dotnet/project-system/blob/master/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Rules/CSharp.ProjectItemsSchema.xaml#L12 and then register it like so: https://github.com/dotnet/project-system/blob/master/src/Targets/Microsoft.CSharp.DesignTime.targets#L26.

jasonmalinowski added a commit to jasonmalinowski/roslyn that referenced this issue May 12, 2017
jasonmalinowski added a commit to jasonmalinowski/roslyn that referenced this issue May 13, 2017
This is a workaround for dotnet/project-system#2160 where
AdditionalFileItemNames isn't quite behaving correctly with the IDE.
This can be reverted once that bug is fixed.
jasonmalinowski added a commit to jasonmalinowski/roslyn that referenced this issue Jun 9, 2017
This is a workaround for dotnet/project-system#2160 where
AdditionalFileItemNames isn't quite behaving correctly with the IDE.
This can be reverted once that bug is fixed.
@davkean
Copy link
Member

davkean commented Jun 15, 2017

Note it turns out that ShowMissingItemTypes capability already controls whether these show up in the tree.

@davkean
Copy link
Member

davkean commented May 23, 2018

I looked into ShowMissingItemTypes capability which will prevent the ever growing additional of items that we need to keep adding. This will implement legacy project system-like behavior where unknown items types in the project will show up and have the same properties as item type.

However, it's not yet ready for us to consume:

It won’t show items directly in the project when there is an item for that item type that comes from props/targets. I can see this resulting in confusing behavior; all your items show up in project, then you add a single common one in Directory.Build.props and then they disappear from projects.

It will currently shows PackageReference/ProjectReference/Reference also in the project. We can’t do an ItemDefinitionGroup with Visible false for these as we respect it for some of them in the Dependencies tree, such as Reference.

Reached out to CPS to see what we can do about this.

We'll turn this on for 15.8.

@panopticoncentral panopticoncentral added Parity-Legacy-Feature Missing features from the legacy project system. and removed Parity-Legacy-Feature Missing features from the legacy project system. labels Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Request for a new feature, or new scenario to be handled. Parity-Legacy-API Missing or behavior differences in APIs from the legacy project system. Parity-Legacy-Feature Missing features from the legacy project system. Triage-Approved Reviewed and prioritized
Projects
None yet
Development

No branches or pull requests

5 participants