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

[API Proposal]: Allow StringSyntaxAttribute to be applied to return values. #76203

Open
Swimburger opened this issue Sep 26, 2022 · 18 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@Swimburger
Copy link

Background and motivation

The addition of StringSyntaxAttribute (#62505) adds syntax highlighting and more to parameters, fields, and properties which is very helpful already.
A lot of methods also construct strings of a specific syntax and return them, which could benefit from the syntax highlighting and tooling of the StringSyntaxAttribute.

API Proposal

Add AttributeTargets.ReturnValue to the list of targets of the StringSyntaxAttribute.

API Usage

[return: StringSyntax(syntax: StringSyntaxAttribute.Xml)]
public static string BuildTwiml(string name) => $"""
    <?xml version="1.0" encoding="utf-8"?>
    <Response>
        <Say>Ahoy {name}!</Say>
    </Response>
    """;

Alternative Designs

No response

Risks

No response

@Swimburger Swimburger added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 26, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 26, 2022
@ghost
Copy link

ghost commented Sep 26, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

The addition of StringSyntaxAttribute (#62505) adds syntax highlighting and more to parameters, fields, and properties which is very helpful already.
A lot of methods also construct strings of a specific syntax and return them, which could benefit from the syntax highlighting and tooling of the StringSyntaxAttribute.

API Proposal

Add AttributeTargets.ReturnValue to the list of targets of the StringSyntaxAttribute.

API Usage

[return: StringSyntax(syntax: StringSyntaxAttribute.Xml)]
public static string BuildTwiml(string name) => $"""
    <?xml version="1.0" encoding="utf-8"?>
    <Response>
        <Say>Ahoy {name}!</Say>
    </Response>
    """;

Alternative Designs

No response

Risks

No response

Author: Swimburger
Assignees: -
Labels:

api-suggestion, area-System.Diagnostics, untriaged

Milestone: -

@tommcdon
Copy link
Member

@stephentoub

@stephentoub
Copy link
Member

@CyrusNajmabadi, would Roslyn be able to handle this?

@Swimburger, your example uses interpolated strings. They already don't get the syntax highlighting and related features you mention. Do you have real examples that don't involve dynamically-generated content?

@CyrusNajmabadi
Copy link
Member

Sure. We could handle this. But we still won't do anything special for interpolated strings as we have no idea how to interpret the content. We also won't do anything with xml as we lack an xml parser that gives things like positions (afaik)

@Swimburger
Copy link
Author

Swimburger commented Sep 26, 2022

Without string interpolation, it would probably be better to store the string in a const, so I don't have another use-case.
The most common scenario would be to do some work, and at the end of the method interpolate strings with the result of the work.
I wouldn't expect this to work for more complex scenarios.

The current implementation already supports string interpolation, at least in the IDE I tried (Rider 2022.3 EAP 1).
Screenshot of C# code. The code passes an interpolated XML string to a method and the string has syntax highlighting for XML.

@CyrusNajmabadi
Copy link
Member

What happens if you have var name = "</Greeting>";?

We could possibly attempt to support a subset of features here. But we very likely could not effectively do diagnostics.

@Swimburger
Copy link
Author

When I do that, the program works without errors or warnings.

When I intentionally malform the XML, it also doesn't complain about it in combination with string interpolation.
The syntax highlighting works which is a big benefit, but the warnings/errors don't.

Screen Shot 2022-09-26 at 4 14 11 PM

With the interpolation, there's still warnings for empty XML nodes, and code completion.

@CyrusNajmabadi
Copy link
Member

When I intentionally malform the XML, it also doesn't complain about it in combination with string interpolation.

Yeah, this makes it just a heuristic. We could consider that, but it's feels like it's full of caveats and can easily make people think code is fine when it's really not.

@Swimburger
Copy link
Author

That makes sense, although the same would apply to the current attribute targets, no?

@CyrusNajmabadi
Copy link
Member

This is solely about interpolated strings. Normal non-interpolated strings are fine and can be correctly analyzed in all contexts.

@svick
Copy link
Contributor

svick commented Sep 27, 2022

Attributes generally tell something about a method to something outside that method. But here, the attribute would be only used inside the method itself. Because of that, I think the // lang=xml syntax, which works today, would be a better fit here.

@tommcdon tommcdon added this to the 8.0.0 milestone Sep 28, 2022
@tommcdon tommcdon removed the untriaged New issue has not been triaged by the area owner label Sep 28, 2022
@tommcdon tommcdon added the enhancement Product code improvement that does NOT require public API changes/additions label Oct 17, 2022
@julealgon
Copy link

But here, the attribute would be only used inside the method itself.

@svick , wouldn't it be possible for the IDE itself to be able to leverage the attribute to display the text in a formatted/syntax-highlighted manner when hovering over a variable for example when in debug mode?

@arakis
Copy link

arakis commented Feb 7, 2023

Because of that, I think the // lang=xml syntax, which works today, would be a better fit here.

Can you explain more about that // lang=xml feature please? I never heard about it. Where to apply that comment?

Edit: It seems to be a JetBrains Rider only feature: https://www.jetbrains.com/help/rider/Language_Injections.html#use-comments
Any plans to integrate that in VS also?

@julealgon
Copy link

Can you explain more about that // lang=xml feature please? I never heard about it. Where to apply that comment?

@arakis It is a comment that allows you to tell the format for a specific string argument/variable. Basically, an alternative to [StringSyntaxAttribute].

image

Edit: It seems to be a JetBrains Rider only feature... Any plans to integrate that in VS also?

It is not Rider-specific. It is already part of VS.

@arakis
Copy link

arakis commented Feb 7, 2023

Amazing, i didn't know that. I never read about that little detail, is there any official documentation (except #62505).

By the way, it seems not to work with xml:

image

@CyrusNajmabadi
Copy link
Member

no xml support has been added.

@julealgon
Copy link

...is there any official documentation (except #62505).

I'm actually interested in the answer to this myself. When I was searching to reply to your post @arakis I also was not able to find anything on MSDN at all. Then only thing I found was that git item.

@stephentoub stephentoub removed this from the 8.0.0 milestone May 1, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 1, 2023
@stephentoub stephentoub added this to the Future milestone May 1, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

8 participants