Corrected impl of typeof #396

JavedNissar · 2020-05-13T04:23:54Z

Corrected impl of typeof to follow specification by using relevant
AST node directly instead of using UnaryOp

This Pull Request fixes #359.

It changes the following:

Adjusts implementation of TypeOf to use Node::TypeOf instead of Node::UnaryOp

JavedNissar · 2020-05-13T04:31:51Z

Does this code look alright? I ask because I'm encountering the following bizarre compile error and I'm wondering if there's an alternative route I can pursue:

error: internal compiler error: src/librustc/ty/steal.rs:34: attempted to read from stolen value

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:875:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.43.0 (4fb7144ed 2020-04-20) running on x86_64-apple-darwin

note: compiler flags: -C debuginfo=2 -C incremental --crate-type cdylib --crate-type lib

note: some of the compiler flags provided by cargo are hidden

thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `LLVMing`,
 right: `Codegenning`', <::std::macros::panic macros>:5:6

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.43.0 (4fb7144ed 2020-04-20) running on x86_64-apple-darwin

note: compiler flags: -C debuginfo=2 -C incremental --crate-type cdylib --crate-type lib

note: some of the compiler flags provided by cargo are hidden

error: aborting due to previous error

error: could not compile `Boa`.

Razican · 2020-05-13T06:29:25Z

That is in fact a bug inside the Rust compiler. You should report it, as the error says. You could also try to upgrade to the latest stable version, to see if they solved the issue.

Razican

Hi! thanks for the contribution. After some discussion in #359, it seems that it's best if we implement this as a unary expression and remove the Node, then move the exec part to the unary expression execution.

We should also add tests to this.

jasonwilliams · 2020-05-13T16:28:59Z

Hi! thanks for the contribution. After some discussion in #359, it seems that it's best if we implement this as a unary expression and remove the Node, then move the exec part to the unary expression execution.

We should also add tests to this.

Yeah sorry about that its my fault i was under the impression it was a Node expression and not a unary one.

Razican

Very nice indeed! This is almost ready to be merged, just give a look to my comments :)

Razican · 2020-05-14T06:43:12Z

boa/src/exec/mod.rs

+                                "object"
+                            }
+                        }
+                    }),
                    _ => unimplemented!(),


It seems that we already implemented all variants, so this is unreachable :D

Suggested change

_ => unimplemented!(),

boa/src/exec/tests.rs

boa/src/syntax/ast/node.rs

Razican

This is ready to be merged from my side :)

HalidOdat

Looks good to me too. Check my comment it's mostly for my understanding.

HalidOdat · 2020-05-15T08:22:45Z

boa/src/exec/mod.rs

+                    UnaryOp::TypeOf => Value::from(match v_a.data() {
+                        ValueData::Undefined => "undefined",
+                        ValueData::Symbol(_) => "symbol",
+                        ValueData::Null => "object",
+                        ValueData::Boolean(_) => "boolean",
+                        ValueData::Rational(_) | ValueData::Integer(_) => "number",
+                        ValueData::String(_) => "string",
+                        ValueData::Object(ref o) => {
+                            if o.deref().borrow().is_callable() {
+                                "function"
+                            } else {
+                                "object"
+                            }
+                        }
+                    }),


Why do we do this here when we can just call value.get_type() which returns a &'static str, it seems like were duplicating code.

I think i mentioned in the original issue that we don't need both jasonwilliams#359 (comment)

HalidOdat

I think it's best to keep the get_type function in value and just call it from exec. there are a lot of places were get_type is called.

jasonwilliams

as @HalidOdat said maybe we can just call get_type() from UnaryOf::TypeOf

Razican

Hi, it seems it's almost ready, just one test failing. Could you check why? I also added a small comment to see if we can improve testing coverage and documentation for null comparison.

Razican · 2020-05-18T07:33:55Z

boa/src/builtins/value/operations.rs

+        //Null has to be handled specially because "typeof null" returns object and if we managed
+        //this without a special case we would compare self and other as if they were actually
+        //objects which unfortunately fails
+        if self.is_null() {
+            return true;
+        }
+


Can we add a test for this, and a link to the spec?

Isn't boa/src/builtins/value/tests.rs:58 already a test for this?

Yes! Perfect, thanks :)

JavedNissar · 2020-05-19T05:23:29Z

I'm having difficulty replicating the errors in CI on my local development environments (Windows and Mac OSX). I'm not entirely sure where to proceed from here so I would love to have some suggestions!

jasonwilliams · 2020-05-20T21:28:16Z

I'm having difficulty replicating the errors in CI on my local development environments (Windows and Mac OSX). I'm not entirely sure where to proceed from here so I would love to have some suggestions!

Looks like you've found a regression introduced by my "implement this" change

I will look at this.

JavedNissar · 2020-05-20T21:30:32Z

I'm having difficulty replicating the errors in CI on my local development environments (Windows and Mac OSX). I'm not entirely sure where to proceed from here so I would love to have some suggestions!

Looks like you've found a regression introduced by my "implement this" change

I will look at this.

Thanks!

jasonwilliams · 2020-05-20T21:53:27Z

Lets see if https://github.com/jasonwilliams/boa/pull/407 passes and ill merge that, String wasn't defaulting to empty when being called as const a = String()

The fact that you made a test for this is great as we may not have captured that before.
Once i merged that you will need to rebase

jasonwilliams · 2020-05-20T22:05:05Z

https://github.com/jasonwilliams/boa/pull/407 merged

Removed Node::TypeOf, implemented UnaryOp::TypeOf, and added tests

HalidOdat

This looks perfect! Thanks for the contribution @restitutororbis :)

Razican

This is good to go for me as well :)

Razican requested changes May 13, 2020

View reviewed changes

Razican added enhancement New feature or request bug Something isn't working parser Issues surrounding the parser labels May 13, 2020

JavedNissar force-pushed the type-of-correction branch 7 times, most recently from c502fc6 to 08760f2 Compare May 14, 2020 03:48

Razican reviewed May 14, 2020

View reviewed changes

Razican added this to the v0.8.0 milestone May 14, 2020

jasonwilliams modified the milestones: v0.8.0, v0.9.0 May 14, 2020

JavedNissar force-pushed the type-of-correction branch from 08760f2 to a74de30 Compare May 15, 2020 02:59

Razican approved these changes May 15, 2020

View reviewed changes

Razican requested review from HalidOdat and jasonwilliams May 15, 2020 06:49

HalidOdat reviewed May 15, 2020

View reviewed changes

JavedNissar force-pushed the type-of-correction branch from a74de30 to 93b5270 Compare May 16, 2020 01:54

JavedNissar requested a review from HalidOdat May 16, 2020 01:55

HalidOdat requested changes May 16, 2020

View reviewed changes

jasonwilliams requested changes May 17, 2020

View reviewed changes

JavedNissar force-pushed the type-of-correction branch from 93b5270 to ca1de5c Compare May 18, 2020 05:21

JavedNissar requested review from jasonwilliams and HalidOdat May 18, 2020 05:21

JavedNissar force-pushed the type-of-correction branch from ca1de5c to 8ede26a Compare May 18, 2020 05:22

Razican reviewed May 18, 2020

View reviewed changes

JavedNissar force-pushed the type-of-correction branch from 8ede26a to 55b4610 Compare May 19, 2020 00:03

jasonwilliams mentioned this pull request May 20, 2020

String wasn't defaulting to empty when called as String() with no arg… #407

Merged

Resolved boa-dev#359

b92dac2

Removed Node::TypeOf, implemented UnaryOp::TypeOf, and added tests

JavedNissar force-pushed the type-of-correction branch from 55b4610 to b92dac2 Compare May 21, 2020 00:09

HalidOdat approved these changes May 21, 2020

View reviewed changes

Razican approved these changes May 21, 2020

View reviewed changes

Razican merged commit 29abfd6 into boa-dev:master May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Corrected impl of typeof #396

Corrected impl of typeof #396

JavedNissar commented May 13, 2020 •

edited

Loading

JavedNissar commented May 13, 2020

Razican commented May 13, 2020 •

edited

Loading

Razican left a comment

jasonwilliams commented May 13, 2020 •

edited

Loading

Razican left a comment

Razican May 14, 2020

Razican left a comment

HalidOdat left a comment

HalidOdat May 15, 2020 •

edited

Loading

jasonwilliams May 15, 2020

HalidOdat left a comment

jasonwilliams left a comment

Razican left a comment

Razican May 18, 2020

JavedNissar May 18, 2020

Razican May 21, 2020 •

edited

Loading

JavedNissar commented May 19, 2020

jasonwilliams commented May 20, 2020

JavedNissar commented May 20, 2020

jasonwilliams commented May 20, 2020 •

edited

Loading

jasonwilliams commented May 20, 2020

HalidOdat left a comment

Razican left a comment

Corrected impl of typeof #396

Corrected impl of typeof #396

Conversation

JavedNissar commented May 13, 2020 • edited Loading

JavedNissar commented May 13, 2020

Razican commented May 13, 2020 • edited Loading

Razican left a comment

Choose a reason for hiding this comment

jasonwilliams commented May 13, 2020 • edited Loading

Razican left a comment

Choose a reason for hiding this comment

Razican May 14, 2020

Choose a reason for hiding this comment

Razican left a comment

Choose a reason for hiding this comment

HalidOdat left a comment

Choose a reason for hiding this comment

HalidOdat May 15, 2020 • edited Loading

Choose a reason for hiding this comment

jasonwilliams May 15, 2020

Choose a reason for hiding this comment

HalidOdat left a comment

Choose a reason for hiding this comment

jasonwilliams left a comment

Choose a reason for hiding this comment

Razican left a comment

Choose a reason for hiding this comment

Razican May 18, 2020

Choose a reason for hiding this comment

JavedNissar May 18, 2020

Choose a reason for hiding this comment

Razican May 21, 2020 • edited Loading

Choose a reason for hiding this comment

JavedNissar commented May 19, 2020

jasonwilliams commented May 20, 2020

JavedNissar commented May 20, 2020

jasonwilliams commented May 20, 2020 • edited Loading

jasonwilliams commented May 20, 2020

HalidOdat left a comment

Choose a reason for hiding this comment

Razican left a comment

Choose a reason for hiding this comment

JavedNissar commented May 13, 2020 •

edited

Loading

Razican commented May 13, 2020 •

edited

Loading

jasonwilliams commented May 13, 2020 •

edited

Loading

HalidOdat May 15, 2020 •

edited

Loading

Razican May 21, 2020 •

edited

Loading

jasonwilliams commented May 20, 2020 •

edited

Loading