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

C# Metadata Questions #3246

Open
InsertCreativityHere opened this issue Dec 9, 2024 · 6 comments
Open

C# Metadata Questions #3246

InsertCreativityHere opened this issue Dec 9, 2024 · 6 comments

Comments

@InsertCreativityHere
Copy link
Member

InsertCreativityHere commented Dec 9, 2024

I've been working on #117 (improve metadata validation).
For my PR, I'm trying to keep the current behavior as much as possible (to avoid changing too much in one PR),
but I've across a few oddities that don't seem right to me. I intend to fix these in a follow-up PR, but wanted to list them here,
in case anyone had any thoughts/explanations to offer.

  1. We support SortedList as an argument to cs:generic on dictionaries:
    This seems nonsensical to me, why would you map a dictionary to a list, instead of using a sequence?
    Also, in the documentation, we only list SortedDictionary, not SortedList.
    EDIT: @pepone points out that SortedList is actually an IDictionary, so we should keep support for it.

  2. We only support cs:implements on structs and interfaces. Why don't we support this on classes?
    Note that in the documentation, we state that it's supported on struct, class, and interface.
    It's not clear if the documentation is outdated, or if this is a bug in the current validation logic.
    EDIT: after looking at it further, this metadata seems useless and will be removed instead.

  3. Should we allow cs:attribute on enumerators?
    If you put this metadata on an enumerator, we don't reject it, but we also don't seem to generate code for it.
    It's just silently ignored. Is there a reason we'd disallow this? Or is this just missing functionality.
    I intend to change slice2cs to generate attributes on enumerators like we do for everything else.

  4. Should we allow cs:property on exceptions?
    The documentation says it's only supported on structs and classes, but the validation logic allowed it through:

    else if (dynamic_pointer_cast<ClassDecl>(cont) || dynamic_pointer_cast<Exception>(cont))
    {
    if (directive == "cs:property" && arguments.empty())
    I intend to allow this metadata on exceptions, unless there's a reason not to

@pepone
Copy link
Member

pepone commented Dec 9, 2024

@pepone
Copy link
Member

pepone commented Dec 9, 2024

I intend to change slice2cs to generate attributes on enumerators like we do for everything else.

Seems the right thing to do.

@InsertCreativityHere
Copy link
Member Author

Should we allow cs:property on exceptions?

Apparently we actually do this in some of our tests, so I'm going to add this to my PR, instead of doing it after-the-fact.
Otherwise, we'd get build warnings on main.

@pepone
Copy link
Member

pepone commented Dec 9, 2024

We support SortedList as an argument to cs:generic on dictionaries:

Because sorted list is a collection of key/value pairs and it implements IDictionary, see https://learn.microsoft.com/en-us/dotnet/api/system.collections.sortedlist?view=net-9.0

@InsertCreativityHere
Copy link
Member Author

Because sorted list is a collection of key/value pairs and it implements IDictionary

Yes, you're right. I should of looked closer at SortedList, although that seems like a pretty bad name to describe what it actually is. But yes, we should keep this support, and we should also make sure to document it somewhere too... It's pretty crazy we don't document or test it anywhere, since we've been supporting it for over 17 years now:
1d27c59#diff-88cc14de1ecb3a23b3990783d7e7c41dcd87f24f315607c3cbb6622dcea5d00c

@InsertCreativityHere
Copy link
Member Author

Currently, we allow cs:attribute on modules, we should remove this since C# doesn't support attributes on namespaces.

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

No branches or pull requests

2 participants