-
-
Notifications
You must be signed in to change notification settings - Fork 691
feat: Parse parameter decorators #3015
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
Closed
PaperPrototype
wants to merge
37
commits into
AssemblyScript:main
from
PaperPrototype:parse-parameter-decorators
Closed
Changes from 28 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
15564e9
Parse and preserve parameter decorators
jtenner 75d7c6a
Reject surviving parameter decorators after transforms
jtenner 65527dd
Cover transform-time removal of parameter decorators
jtenner 32f69b2
Clarify transform-only parameter decorator contract
jtenner 21f724f
Document transform hook timing for parameter decorators
jtenner c474861
Fix lint for parameter decorator fixtures
jtenner ec618c9
chore: ignore AS parameter decorator fixture in eslint
jtenner 4505b5c
Fix parameter decorator review feedback
jtenner c267d6e
chore: remove legacy eslint config
jtenner 6f75b5e
refactor: consolidate parameter decorator validation
jtenner 53dc57c
chore: remove unused AST imports
jtenner bcbce9a
style: move parameter range to the end
jtenner ba37601
style: include explicit this decorators in function types
jtenner b1fa8aa
refactor: reuse decorator serialization for parameters
jtenner 2a7f4fb
refactor: rename tryParseParameterDecorators helper
jtenner e401a30
Refactor parameter decorator parsing
jtenner d1ce4c7
Update NOTICE
PaperPrototype 2ca7c6a
Update NOTICE
PaperPrototype 93f7b31
undo unecessary comment changes
PaperPrototype 9757397
Undo changes to index-wasm.ts
PaperPrototype 2177f77
removed currentSourceStatementDepth and currentSourceStatementHasPara…
PaperPrototype 9b2dc22
Remove NodeWalker and ParameterDecoratorValidator and validateParamet…
PaperPrototype 9f9f3bd
Remove NodeWalker from ast.ts
PaperPrototype 6743001
remove validateParameterDecorators from compiler.ts
PaperPrototype efe656b
Update compiler.ts
PaperPrototype 3e8b163
Update compiler.ts
PaperPrototype 516b2ec
Some claude stuff
PaperPrototype 6615b02
undo tab in compiler.ts
PaperPrototype 0e68e6a
Create temporary TODO.md for myself to write things down
PaperPrototype 4afd421
Update TODO.md
PaperPrototype 056ef1f
Transformer hook notes in TODO.md
PaperPrototype bc7f01e
Remove validation from compiler
PaperPrototype bcaacc6
Write test transformer in typescript to import types from assemblyscr…
PaperPrototype 4deaa00
Update TODO.md
PaperPrototype ca13f28
Cleanup
PaperPrototype a98ec09
everything should be working now
PaperPrototype 89290ae
fix `npm run bootstrap:debug`
PaperPrototype File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t like that transformer validation happens in the compiler. If it’s not valid for upstream AS semantics, it’s better to move it to transform utils or add it to their pipeline automatically.
TS will not merge decorators soon, and even if it does, parameters are still unclear:
https://github.com/tc39/proposal-decorators/blob/master/EXTENSIONS.md#parameter-decorators-and-annotations
If param decorator semantics are not standardized there either, it feels wrong to validate this inside our compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I’d just pass the bare minimum of what needs to be fed into the transformer, and let the transformer itself parse, canon and validate the decorators via the AST. Especially considering that this feature is pretty niche in terms of general use.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I have no idea what I am doing 😅
Good to know about the decorators proposal for TS, I wasn't aware of it before 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "add it to their pipeline automatically"?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean extend transform's API by adding new validation entry point here between listFiles and afterParse and add validation hook to this entry point instead validate by itself. This would allow users to validate the code in the transformer themselves before compilation but after parsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I needed to reload the page before I saw this. When you say "Transformer" what are you referring to? I'm assuming you mean the implementors who will want to make use of these decorators? Eg. the example transformers in /tests/transform? Still very new to all the verbiage being thrown around here, sorry about the extreme noobiness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh unless you are referring to the AssemblyScript AST transformer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I finally get what you meant. I made some notes:
Is this correct? Just a "👍" is fine for me, unless I am completely wrong 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's pretty accurate