fix(hir): #832 — class decorator identity-return (return target) accepted as no-op#840
Merged
Merged
Conversation
…cepted as no-op `append_decorator_invocations_inner` (PR #754) threw on any non-undefined decorator return value to surface the silent class-replacement-not-installed case. That gate was too strict — many real-world decorators (TypeORM, NestJS, GraphQL wrappers, any factory output that hasn't been simplified for production) end with `return target` as a benign no-op, and the trivial identity decorator from the bug report was rejected as well. Augment the condition to accept identity returns: `if (__ret !== undefined && __ret !== target) throw ...`. The target reference comes from `invocation_args.first()` — the `Expr::ClassRef` that `append_class_decorator_invocations` passes as the decorator's first argument. Genuine class replacement (`return SomeOtherClass`) still throws with the same actionable message, preserving the #754 safety guarantee. Closes #832.
1b224af to
3362228
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@decorator class X {}where the decorator returns the target class (return target) no longer throwsTypeError: ... returned a value. Identity-return is treated as a no-op.return SomeOtherClass) still throw with the same actionable message, preserving the safety guarantee from PR Add legacy TypeScript decorator metadata support #754.Root cause
append_decorator_invocations_innerincrates/perry-hir/src/lower.rsemittedif (__ret !== undefined) throw TypeError(...)— strict on any non-undefined return. Real-world decorators (TypeORM@Entity, NestJS, GraphQL wrappers, decorator-factory output not simplified for production) routinely end withreturn target/return descriptoras a no-op, and the trivial identity decorator from the bug report was rejected too.Fix
Augment the condition to also accept identity returns:
targetcomes frominvocation_args.first()— theExpr::ClassRef(class.name)thatappend_class_decorator_invocationspasses as the decorator's first argument. So the runtime comparison is between the decorator's return value and the original class reference it received. Falls back to the strict-undefined check ifinvocation_argsis empty (defensive).Test plan
Hello, world—@logged(returns target) lets the class instantiate and the method dispatch correctly.@replaceClass(returns a different function) still throws the TypeError. The Add legacy TypeScript decorator metadata support #754 silent-class-replacement safety guarantee is preserved.test_gap_console_methodsandtest_gap_regexp_advanced/ String.prototype.replace(/.../g, fn) — replacer-fn return value is dropped (replaces with empty string) #833, both unrelated).Closes #832. Refs #793, #801, #754.