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

Optimize attribute checking #16036

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Optimize attribute checking #16036

merged 7 commits into from
Oct 10, 2023

Conversation

kerams
Copy link
Contributor

@kerams kerams commented Sep 24, 2023

TcTypeAndRecover would redo ResolveTypeLongIdent, so we bypass duplicate work and skip directly to TcTypeApp. This has the additional benefit that we no longer capture 2 Item.Types name resolutions for each attribute (one with ItemOccurence.UseInAttribute and the other ItemOccurence.UseInType) but only the first one.

@kerams kerams requested a review from a team as a code owner September 24, 2023 08:39
@kerams
Copy link
Contributor Author

kerams commented Sep 24, 2023

I also wonder whether CheckTyconAccessible cenv.amap m env.AccessRights tcref in TcTypeApp is strictly necessary. tcref will almost certainly have come from ResolveTypeLongIdent or similar, which should perform accessibility checks. Not sure how safe it would be to get rid of this.

@smoothdeveloper
Copy link
Contributor

@kerams, wondering if we could also optimize which lookup to try first, if the text already ends with Attribute, 99.99% of cases it would be better to try the one without appending it.

I can only see weird stuff like AttributeAttribute that would require the secondary lookup then.

@kerams
Copy link
Contributor Author

kerams commented Sep 24, 2023

That would give a different meaning to the program if you have SomeAttributeAttribute and SomeAttribute in scope.

@smoothdeveloper
Copy link
Contributor

@kerams, ok makes sense and it is a bit sad edge case :(

I'm wondering if TcTypeMeasureApp is equivalent to your TcTypeApp? does it still type check measures annotated values passed to an attribute?

open FSharp.Data.UnitSystems.SI.UnitNames
type AttAttribute(a: int<second>) =               
  inherit System.Attribute()

[<Att(1)>]             
type Foo() =
  member x.X = 1

@vzarytovskii
Copy link
Member

This will have to wait unfortunately, net8 is now only open for fixes.

@vzarytovskii
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro T-Gro enabled auto-merge (squash) October 9, 2023 17:28
@T-Gro T-Gro merged commit 7f037b7 into dotnet:main Oct 10, 2023
@kerams kerams deleted the a branch October 10, 2023 15:03
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.

6 participants