Skip to content

JS: Adding model for .get function of Map in Unvalidated Dynamic Method Call#7828

Merged
erik-krogh merged 6 commits into
github:mainfrom
Naman-ntc:main
Feb 4, 2022
Merged

JS: Adding model for .get function of Map in Unvalidated Dynamic Method Call#7828
erik-krogh merged 6 commits into
github:mainfrom
Naman-ntc:main

Conversation

@Naman-ntc

Copy link
Copy Markdown
Contributor

Fixes the first part of #7803
Thanks to @max-schaefer for suggesting the dataflow alternative of enhancement I proposed
Summary of changes -

  • Introduced a model for .get function of Map
  • Added more test files

@Naman-ntc Naman-ntc requested a review from a team as a code owner February 3, 2022 09:32
@github-actions github-actions Bot added the JS label Feb 3, 2022
app.get('/perform/:action/:payload', function(req, res) {
let action = actions.get(req.params.action);
// GOOD: `action` is either the `play` or the `pause` function from above
if (typeof action === 'function') {

@Naman-ntc Naman-ntc Feb 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should not be flagged because action cannot be from proto right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That sounds right, a call to get on a map shouldn't be able to return something from the prototype.

I've seen a bunch of get implementations that are implemented like function get(x) {return obj[x];}, but if that's the case, then the query should pick up on the dynamic property read inside the get function.

@Naman-ntc Naman-ntc changed the title Fixing model for .get function of Map Adding model for .get function of Map in Unvalidated Dynamic Method Call Feb 3, 2022
@owen-mc owen-mc changed the title Adding model for .get function of Map in Unvalidated Dynamic Method Call JS: Adding model for .get function of Map in Unvalidated Dynamic Method Call Feb 3, 2022
@erik-krogh erik-krogh self-assigned this Feb 4, 2022

@erik-krogh erik-krogh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍
Good work!

Just two comments about the placement of the // OK comments.

Naman-ntc and others added 2 commits February 4, 2022 16:12
…amicMethodCallGood4.js

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
…amicMethodCallGood3.js

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
@erik-krogh

Copy link
Copy Markdown
Contributor

And my suggestions made it so the UnvalidatedDynamicMethodCall.qlref test is failing.
You need to run the test and update the expected output.

@Naman-ntc

Copy link
Copy Markdown
Contributor Author

Ah, just realized that line numbers changed! Yes, will fix the expected files!

@erik-krogh erik-krogh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

I'll do a small evaluation to check if there is a performance regression (I highly doubt that), and then I'll merge it.

@erik-krogh erik-krogh merged commit ab2d3a7 into github:main Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants