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

DeclarationKind is missing Module #10094

Closed
heejaechang opened this issue Mar 25, 2016 · 14 comments
Closed

DeclarationKind is missing Module #10094

heejaechang opened this issue Mar 25, 2016 · 14 comments
Assignees
Labels

Comments

@heejaechang
Copy link
Contributor

MS.CA.Editing.DeclarationKind is missing Module. so any method that deals with DeclarationKind can't handle VB Module case.

@CyrusNajmabadi
Copy link
Member

Isn't this by design? The workspace editing APIs are for when you want to generate code in a common manner over VB and C#. C# doesn't have modules, so you wouldn't use this layer to generate such things. If you just want to generate modules, why not just actually use the VB syntax model?

@CyrusNajmabadi
Copy link
Member

Giving back to heejae. This seems by design to me. 'Modules' are a VB specific concept. These APIs are for giving an abstrcation of the intersection of C# and VB. If you need to deal with modules, shouldn't you be writing VB specific code? Can you give an example of what you're trying to do, and why you need support for this at the CA.Editing layer?

@heejaechang
Copy link
Contributor Author

@CyrusNajmabadi are you saying people shouldn't use SyntaxGenerator at all? or instead use VBSyntaxGenerator? first language specific SyntaxGenerator is not exposed. and the way SyntaxGenerator works and how it all connected with SyntaxEditor are different than SyntaxFactory and convenience baked into those code generator.

I think we should either expose language specific SyntaxGenerator or put language specific things in common layer for syntax generator.

it is weird asking people to learn 2 different way to generate code especially people who doesnt live in compiler layer but in workspace layer for added convenience.

@heejaechang
Copy link
Contributor Author

@srivatsn @Pilchie @mattwar adding other people for their opinion.

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi are you saying people shouldn't use SyntaxGenerator at all?

People shoudl not use SyntaxGenerator if they're creating something that is language specific. SyntaxGenerator is for abstacting over VB and C# so you can generate properly for both languages. If you're doing VB specific stuff you should just use the VB SyntaxFactory API.

@heejaechang
Copy link
Contributor Author

@CyrusNajmabadi that seems a bit trouble some. since that kind of subtle thing user will be confused. once people went down on using SyntaxGenerator, they probably will want to use it for all rather than switching to different shape of APIs.

also, this kind of API in syntax generator
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Editing/SyntaxGenerator.cs,72

to be useful then, I think it needs to consider language specific thing as well. since it won't recognize module as declaration.

@mattwar
Copy link
Contributor

mattwar commented Sep 6, 2016

It would be okay to include Module in DeclarationKind. It can be a super set.

It would not work to add a Module factory API on SyntaxGenerator though.

@CyrusNajmabadi
Copy link
Member

since that kind of subtle thing user will be confused. once people went down on using SyntaxGenerator, they probably will want to use it for all rather than switching to different shape of APIs.

We already have had to deal with this numerous times. When the languages diverge we use language specific solutions. Only where they share do we use the unified APIs.

@heejaechang
Copy link
Contributor Author

heejaechang commented Sep 6, 2016

@CyrusNajmabadi yes. we did that many times and what I am saying is doing that is not that intuitive. once one went down the path of using SyntaxEditor/SyntaxGenerator staying on that API seems more intuitive.

@heejaechang
Copy link
Contributor Author

@Pilchie anyway, I will let IDE team decide what is right API. if IDE team think current API is good enough. just close the bug :)

@heejaechang
Copy link
Contributor Author

by the way, if we are going to ask users to use compiler API (SyntaxFactory) for any language specific construct, shouldn't SyntaxGenerator at the compiler layer? when I tried to use SynaxGenerator in code fix for FxCop, it fall short quite quickly since such common thing as GetDecleration won't work with VB.

after that, another thing got in the way was, I was forced to use compiler layer API with workspace layer API, I had to first look at the source of SyntaxGenerator code to make sure two can be used together (to make sure SyntaxGenerator doesn't do some magic underneath).

it was just more overhead to use the API. if SyntaxGenerator is simply wrapper on top of SyntaxFactory, then we could remove SyntaxGenerator and have CommonSyntaxFactory which has bunch of abstract methods like SyntaxGenerator and then, user can cast it to VB or CSharpSyntaxFactory and use consistent API for language specific constructs without worrying about implementation detail first.

if SyntaxGenerator provides more than what SyntaxFactory does, then user should be able to keep using that stuff for language specific construct by simply casting SyntaxGenerator to VB or CSharpSyntaxGenerator rather than mix use compiler and workspace APIs.

@CyrusNajmabadi
Copy link
Member

shouldn't SyntaxGenerator at the compiler layer

It could have been. But moving that now would be a breaking change. Also, SyntaxGenerator does extra stuff that the compiler layer doesn't know how to do. Moving that down would be difficult.

@CyrusNajmabadi
Copy link
Member

rather than mix use compiler and workspace APIs.

I don't get this point. Nearly everything at the feature level uses the workspace and compiler APIs. It's the norm. The compiler API generally serves as the raw source of data. And the workspace APIs often serve as the way to operate over that data in a uniform fashion when that data is common to both C# and VB.

There is no expectation that someone be able to just use the compiler API. If we wanted that, we'd have to move a ton of code down to that layer (formatting, simplification, etc.). We could potentially do that in the future. But we decided (literally years) ago that we'd have this layering. I've heard nothing that would indicate that we'd change this (let alone deal with how to avoid the breaking change problem).

@CyrusNajmabadi
Copy link
Member

Closing this because the intent is that you use SyntaxGenerator when operating generically over Vb and C#. If you're doing VB specific stuff then you should use the existing features that allow you to target VB directly.

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

No branches or pull requests

5 participants