Skip to content

Commit

Permalink
inherit @:unreflective on generic classes
Browse files Browse the repository at this point in the history
  • Loading branch information
Simn committed Apr 23, 2024
1 parent db842bf commit 68c95df
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/typing/generic.ml
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ let build_generic_class ctx c p tl =
| Pure
| Struct | StructInit
| Using
| AutoBuild ->
| AutoBuild | Unreflective ->
true
| _ ->
false
Expand Down

6 comments on commit 68c95df

@kLabz
Copy link
Contributor

@kLabz kLabz commented on 68c95df Jun 28, 2024

Choose a reason for hiding this comment

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

@Simn I'm adding this to 4.3_bugfix, which doesn't have AutoBuild (nor KeepSub) there.
Should I add all 3, or only Unreflective? (given that the other two were added in a much bigger change (#11376))

@Simn
Copy link
Member Author

@Simn Simn commented on 68c95df Jun 28, 2024

Choose a reason for hiding this comment

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

Mmh, I'm not sure about AutoBuild because that might cause behavioral differences. I'd dig up why this was added to see if it's a worthwhile bugfix for 4.3.

@kLabz
Copy link
Contributor

@kLabz kLabz commented on 68c95df Jun 28, 2024

Choose a reason for hiding this comment

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

Added there d2fa580

@Simn
Copy link
Member Author

@Simn Simn commented on 68c95df Jun 28, 2024

Choose a reason for hiding this comment

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

I'd skip it, #5536 is a narrow (and old) issue and the involvement of #11376 makes me rather nervous.

@kLabz
Copy link
Contributor

@kLabz kLabz commented on 68c95df Jun 28, 2024

Choose a reason for hiding this comment

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

Sounds good 👍

What about 1200c5c / #6500 for @:keepSub? Seems like the same arguments apply there

@Simn
Copy link
Member Author

@Simn Simn commented on 68c95df Jun 28, 2024

Choose a reason for hiding this comment

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

I feel like with DCE the worst case is that something ends up being kept that shouldn't be kept, which isn't that terrible. So I'd include that one.

Please sign in to comment.