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

Allow generic attributes #14714

Closed
wants to merge 1 commit into from
Closed

Allow generic attributes #14714

wants to merge 1 commit into from

Conversation

kerams
Copy link
Contributor

@kerams kerams commented Feb 7, 2023

@vzarytovskii
Copy link
Member

Amazing!

Comment on lines +1019 to +1025
LessRange: range option

TypeArgs: SynType list

CommaRanges: range list

GreaterRange: range option
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a Trivia node that @nojaf might have opinions on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, SynType.App, SynType.LongIdentApp and SynExpr.TypeApp use those 3 range fields too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SynType.App, SynType.LongIdentApp and SynExpr.TypeApp pre-date SynTrivia, I think for a new concept it make sense to have these as trivia.
It might also make sense to introduce a new type for TypeName : SynLongIdent.

@kerams
Copy link
Contributor Author

kerams commented Feb 7, 2023

[<NoEquality; NoComparison; StructuredFormatDisplay("{DebugText}")>]
type Attrib =
| Attrib of
tyconRef: TyconRef *
kind: AttribKind *

This is a problem since we're working with TType now. Do I store both? And what about things like remapAttribImpl? Is it safe to replace just the tycon in TType_app?

Ah, and most importantly, typed tree pickling works with attribute tycons.

I'm also currently forced to do [<Foo<string> >] because the parser doesn't like >>]. How would I go about sorting that out?

Comment on lines +1158 to 1169
/// The syntax range of the LESS symbol
LessRange: range option

/// Type arguments
TypeArgs: SynType list

/// Ranges of the commas between type arguments
CommaRanges: range list

/// The syntax range of the GREATER symbol
GreaterRange: range option
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to duplicate info from SynType.App, SynType.LongIdentApp and SynExpr.TypeApp. Maybe we could extract these field into a separate type, like SynTypeArgumentList or something?

Copy link
Contributor Author

@kerams kerams Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the question is whether we extract all 4 as SynTypeArgumentList or just the ranges as SynTypeArgumentTrivia, or

type SynTypeArgumentTrivia = {
      LessRange: range option
      CommaRanges: range list
      GreaterRange: range option
}

type SynTypeArgumentList = {
    TypeArgs: SynType list
    Trivia: SynTypeArgumentTrivia
}

src/Compiler/SyntaxTree/SyntaxTree.fsi Show resolved Hide resolved
@kerams kerams closed this Jul 20, 2023
@kerams kerams deleted the attgen branch August 14, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants