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

Force game functions var args #220

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Conversation

gok99
Copy link
Contributor

@gok99 gok99 commented Sep 4, 2023

Description

Module function var-args has been broken for awhile. Let's add a quick fix by injecting game functions minArgsNeeded

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Load modules and check that var args are allowed

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@gok99 gok99 requested review from leeyi45 and lhw-1 September 8, 2023 08:35
Copy link

@lhw-1 lhw-1 left a comment

Choose a reason for hiding this comment

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

In conjunction with this PR, the changes seem to have fixed the erroneous behavior for "my room". Thanks for the PR!

Will leave it to the modules team to approve & merge just in case.

@leeyi45
Copy link
Contributor

leeyi45 commented Sep 9, 2023

Before we go ahead with this, I do think it is worth looking into why the varargs problem exists, and what we can do to fix it for all modules without having to monkey patch things.

Unless this is really urgent in which case we can move forward with it first.

@gok99
Copy link
Contributor Author

gok99 commented Sep 9, 2023

I do agree - we'll only need this by week 7, so there is some time to do something more comprehensive.

As far as I can tell, there seems to be no good way to get information about default parameters from a Function object. So, the arity checks in js-slang will require exactly the number of required parameters to be provided:

https://github.com/source-academy/js-slang/blob/2da5bd620e4af28c6dec516335a93c69bf424ad6/src/utils/operators.ts#L108-L118

For some context, I think module varargs used to work properly (?) long ago (probably before checks of this kind were introduced with rest/spread). When things broke, it started requiring all arguments (including those with default values) - I don't have much insight as to why. It now seems like that behavior has changed and exactly the required arguments must be provided.

@martin-henz
Copy link
Member

I think we can move forward with solving the room issue and at the same time keep a general solution in mind, using a new Issue.

Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

OK, as discussed in telegram group "SA 2425 Game".

@martin-henz martin-henz merged commit e80cf02 into source-academy:master Sep 18, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants