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

Implement String prototype methods #13

Closed
30 of 33 tasks
jasonwilliams opened this issue Apr 4, 2019 · 14 comments
Closed
30 of 33 tasks

Implement String prototype methods #13

jasonwilliams opened this issue Apr 4, 2019 · 14 comments
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@jasonwilliams
Copy link
Member

jasonwilliams commented Apr 4, 2019

String prototype methods

These methods would be added to the String prototype object which is implemented here:
https://github.com/boa-dev/boa/blob/master/boa/src/builtins/string/mod.rs

Spec: https://tc39.es/ecma262/#sec-properties-of-the-string-prototype-object

Implementation Example?
See charAt() and charCodeAt() for a good example

  • charAt()
  • codePointAt()
  • charCodeAt()
  • concat()
  • endsWith()
  • includes()
  • indexOf()
  • lastIndexOf()
  • localeCompare()
  • match()
  • matchAll()
  • normalize()
  • padEnd()
  • padStart()
  • repeat()
  • replace()
  • replaceAll()
  • search()
  • slice()
  • split()
  • startsWith()
  • substring()
  • toLocaleLowerCase()
  • toLocaleUpperCase()
  • toLowerCase()
  • toString()
  • toUpperCase()
  • trim()
  • trimEnd()
  • trimStart()
  • toString()
  • valueOf()
  • @@iterator
@jasonwilliams jasonwilliams added good first issue Good for newcomers help wanted Extra attention is needed labels Apr 4, 2019
@jasonwilliams jasonwilliams changed the title Implement String constructor Implement String constructor Methods Apr 13, 2019
@jasonwilliams jasonwilliams changed the title Implement String constructor Methods Implement String prototype Methods May 7, 2019
@jasonwilliams jasonwilliams changed the title Implement String prototype Methods Implement String prototype methods May 7, 2019
@calluw
Copy link
Contributor

calluw commented Jun 23, 2019

I have started to write some implementations for concat() and repeat() but have encountered an issue: currently none of the String prototype methods work, even existing charAt and charCodeAt:

Using:

let a = "Hello world".chatAt(2);
console.log(a);

Results in the buffer holding only: "Error: undefined"

Is this a known issue or needs to be resolved before prototype methods can be added?

@jasonwilliams
Copy link
Member Author

jasonwilliams commented Jun 23, 2019

Hey @CallumQuick hmm that’s interesting

could you try doing new String(“Hello world”) instead of “Hello world”.charAt() ? I think that may work.

When you make a string literal and immediately use a string prototype method on it, you’re using a string object prototype method on a literal string. Engines will box that value into a String object, Boa doesn’t do this yet, so that will most likely be the reason it doesn’t work.

There is still some work to do here around boxing and unboxing and the best approach.

https://github.com/getify/You-Dont-Know-JS/blob/master/types%20%26%20grammar/ch3.md#boxing-wrappers

@calluw
Copy link
Contributor

calluw commented Jun 23, 2019

Thanks @jasonwilliams, generating the string via new String(..) is working with charAt(), charCodeAt(). Will now test with my new implementations.

@calluw
Copy link
Contributor

calluw commented Jun 23, 2019

On the topic of the boxing of the primitive string, the only current issue with concat(), repeat(), slice() and the other methods that should return strings is that through to_value() they come out as "string literals" again. This means you can't do the following:

let a = new String("Hello world");
a.concat("! ").repeat(2);

Is there an easy way to cause concat, repeat etc to return not just string "values" but string objects? Would this be solved by auto-boxing string literals (#29)?

@jasonwilliams
Copy link
Member Author

jasonwilliams commented Jun 23, 2019

Now you're asking the interesting questions!
We can't do this because the return type of those methods are string types not objects.
Those strings would need to be re-boxed again (yep this is why boxing/un-boxing is a big performance hit and engines often try to find ways around it).

It would be against the spec to return a String instance instead of a string literal.
https://tc39.es/ecma262/#sec-string.prototype.concat

Would this be solved by auto-boxing string literals (#29)?

Basically yes. I think some research would need to be done in this area as i believe there are ways to optimise this operation rather than simply boxing and unboxing again.
In theory we can detect a method being called on a string, so at that point we could convert into a String instance call the method then throw away the instance (wasteful).

There is 1 place where i break this rule and just give back the value, that is when calling length on a string literal. https://github.com/jasonwilliams/boa/blob/master/src/lib/js/value.rs#L202-L208

@calluw
Copy link
Contributor

calluw commented Jun 24, 2019

@jasonwilliams Thanks for clarifying the spec's position (I had overlooked the subtlety of the concat/repeat/slice methods returning values only).

As a heads up, next I will probably take a look at the set of these methods which take a "searchString" argument (so not patterns yet): startsWith(), endsWith(), includes(), indexOf() and lastIndexOf().

jasonwilliams pushed a commit that referenced this issue Jun 30, 2019
…type methods (#13) (#31)

* New concat() and repeat() String prototype implementations

Created new implementations for the JS String prototype which are added
in the string.rs file. Currently not tested.

* Add additional implementation of slice()

* Fix typo which mixed up start and end of slice

* Simplify slice() implementation

* Implementation of String.prototype.startsWith()

* Add implementation of endsWith()

* Implementation of String.prototype.includes()

* Implementation of String.prototype.{indexOf, lastIndexOf}()

* Fix merge conflict missing closing brace
@jasonwilliams jasonwilliams removed the good first issue Good for newcomers label Oct 8, 2019
@Razican Razican added the enhancement New feature or request label Apr 3, 2020
@brian-gavin
Copy link
Contributor

Hey, the links in this issue are out of date due to some code reorg!

Current location of String is here

@Razican
Copy link
Member

Razican commented Jun 1, 2020

Hey, the links in this issue are out of date due to some code reorg!

Current location of String is here

Good catch! I updated the links, but note that some of this is probably blocked until #419 lands :)

@HalidOdat
Copy link
Member

HalidOdat commented Oct 2, 2020

@joshwd36 has implemented String.prototype[@@Symbol.iterator] in #704 , keeping track who has implemented what :)

@HalidOdat HalidOdat added the builtins PRs and Issues related to builtins/intrinsics label Oct 5, 2020
@RageKnify
Copy link
Member

Added split(), being worked on in #1026

@neeldug
Copy link
Contributor

neeldug commented Jun 28, 2021

@jasonwilliams I believe this list is missing replaceAll.

neeldug added a commit to neeldug/boa that referenced this issue Jun 28, 2021
- adds string.prototype.normalize
- uses `unicode_normalization` crate

Closes boa-dev#13
neeldug added a commit to neeldug/boa that referenced this issue Jun 29, 2021
- match method adheres to spec
- regexp method simplified to follow spec

Closes boa-dev#13
neeldug added a commit to neeldug/boa that referenced this issue Jun 29, 2021
neeldug added a commit to neeldug/boa that referenced this issue Jun 29, 2021
neeldug added a commit to neeldug/boa that referenced this issue Jun 29, 2021
@Razican Razican closed this as completed in 09efb2e Jul 1, 2021
@RageKnify
Copy link
Member

@neeldug there's still a few methods in the check list, should this issue be closed?

@neeldug
Copy link
Contributor

neeldug commented Jul 2, 2021

Shouldn’t be closed, only checked off.

@jedel1043
Copy link
Member

Closing in favour of #1562.

bors bot pushed a commit that referenced this issue Feb 15, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccesary.
--->

This Pull Request fixes existing string prototype methods in #13 and adds static methods.

It changes the following:

- Fix bugs in existing string prototype methods and improve readability (e.g. rename variables to match the names in spec)
- Add static methods `String.raw`, `String.fromCharCode`, `String.fromCodePoint`
- Fix broken unit tests


Co-authored-by: RageKnify <RageKnify@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

8 participants