Make OSError subclass instead of itself #2960

ChJR · 2021-08-24T17:57:38Z

This PR will fix #2914.

# Conflicts: # vm/src/exceptions.rs

vm/src/exceptions.rs

youknowone · 2021-08-26T05:43:15Z

vm/src/exceptions.rs

@@ -581,6 +583,126 @@ macro_rules! extends_exception {
    };
 }

+macro_rules! extends_os_error {


I dont' think we need an own macro for this error. We can write it without macro or using new macro #2897

I need to use tp_new for OSError itself. So I will change it without macro.

youknowone · 2021-08-26T13:29:48Z

@sobolevn Could you look into this PR? This PR is a sort of use case of exception macro. It maybe helpful to give you an idea to decide to choose one.

sobolevn

Looks like that this use-case fits perfectly into

RustPython/vm/src/exceptions.rs

Lines 646 to 657 in c57285b

    
           macro_rules! extend_exception { 
        
               ( $ctx:expr, $class:expr, { $($name:expr => $value:expr),* $(,)* }) => { 
        
                   $class.set_str_attr("__new__", $ctx.new_method("__new__", $class.clone(), PyBaseException::tp_new)); 
        
                   $class.set_str_attr("__init__", $ctx.new_method("__init__", $class.clone(), PyBaseException::init)); 
        
                   extend_class!($ctx, $class, { 
        
                       $( 
        
                           $name => $value, 
        
                       )* 
        
                   }); 
        
               }; 
        
           }

We can re-assign slots with it, but it needs a little polish.
I will send a PR soon! Later we can rebase this one and merge it 👍

Thanks!

# Conflicts: # vm/src/exceptions.rs

# Conflicts: # Cargo.lock # vm/src/exceptions.rs

youknowone · 2021-10-02T08:24:56Z

vm/src/exceptions.rs

+    fn base_exception_new(cls: PyTypeRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult {
+        PyBaseException::slot_new(cls, args, vm)
+    }


if these methods are not required to be moved, keep them on the original place. they will be put in impl later once we revise pyexception macro

They are used in define_macro.

I agree it is used. I don't see a good reason why it need to be moved to here. I checked out the code and tried to compile it without moving this function. I couldn't see any problem.

I see. I moved functions just for distinguishing functions and macros. For readability, if you want not moving the functions I can revert that.

youknowone · 2021-10-04T07:43:20Z

vm/src/lib.rs

@@ -85,6 +85,8 @@ pub use rustpython_common as common;
 #[cfg(feature = "rustpython-compiler")]
 pub use rustpython_compiler as compile;

+use crate::stdlib::errno::errors;


why is this module imported here? I expect this is imported from os_error_optional_new.

My mistake.

youknowone · 2021-10-04T07:43:51Z

vm/src/exceptions.rs

+    fn base_exception_new(cls: PyTypeRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult {
+        PyBaseException::slot_new(cls, args, vm)
+    }


I agree it is used. I don't see a good reason why it need to be moved to here. I checked out the code and tried to compile it without moving this function. I couldn't see any problem.

This reverts commit 73795e8.

This reverts commit 02946e8.

youknowone · 2021-10-07T17:34:13Z

Thank you for contributing! Instead of merge, please try to use rebase next time.

ChJR added 4 commits August 24, 2021 02:45

Make OSError subclass instead of itself

d540bee

Differentiate making OSError and its subclasses

041c2c3

Enable matched test

65fba63

Merge branch 'main' into feature/OSError-Subclass-Ctor

3059bbd

# Conflicts: # vm/src/exceptions.rs

youknowone reviewed Aug 26, 2021

View reviewed changes

vm/src/exceptions.rs Outdated Show resolved Hide resolved

youknowone reviewed Aug 26, 2021

View reviewed changes

Add Windows specific errno OSError subclasses

b9bdaba

Remove OSError specific macro

6eb77ed

sobolevn reviewed Aug 26, 2021

View reviewed changes

sobolevn mentioned this pull request Aug 26, 2021

All exceptions are now modified with extend_exception! macro #2974

Merged

ChJR added 13 commits August 27, 2021 02:34

Merge branch 'main' into feature/OSError-Subclass-Ctor

41e8b31

Fix CI build error on Windows

58ba136

Avoid CI build error on WASI

8ccdef9

Use wasmer_wasi on WASI to handle errno

9a28183

Merge branch 'main' into feature/OSError-Subclass-Ctor

cf61513

Fix typo

27d8b5f

Fix typo again

18a80a8

Merge branch 'main' into feature/OSError-Subclass-Ctor

84bc910

# Conflicts: # vm/src/exceptions.rs

Refine OSError.__new__

0354a71

Merge branch 'main' into feature/OSError-Subclass-Ctor

9250caa

Avoid test failures

5a06415

Merge branch 'main' into feature/OSError-Subclass-Ctor

15877d9

# Conflicts: # Cargo.lock # vm/src/exceptions.rs

Merge branch 'main' into feature/OSError-Subclass-Ctor

4129d74

youknowone reviewed Oct 2, 2021

View reviewed changes

ChJR added 5 commits October 2, 2021 19:05

Merge branch 'main' into feature/OSError-Subclass-Ctor

3ff1025

Avoid CI error

1e38c17

Revert Avoid CI error

2cb27d0

Merge branch 'main' into feature/OSError-Subclass-Ctor

aeeadb1

Use crate::stdlib::errno::errors instead of implementing all constants

124cc27

Remove unused wasmer_wasi on WASI

b90d12a

youknowone requested changes Oct 4, 2021

View reviewed changes

Move 'use crate::stdlib::errno::errors' to proper place

f2fff18

ChJR requested a review from youknowone October 5, 2021 13:38

ChJR added 8 commits October 5, 2021 22:43

Use direct methods rather than wrapper methods

ea5cdf2

Merge branch 'main' into feature/OSError-Subclass-Ctor

e0d88ed

Add errnos on WASI

fa1718b

Remove cfg over crate::stdlib::errno:errors module

02946e8

Add errno::errors on other platforms

73795e8

Merge branch 'main' into feature/OSError-Subclass-Ctor

a23101b

Avoid CI build error

2572890

Avoid WASI build error by not using errno::errors

a504a74

ChJR force-pushed the feature/OSError-Subclass-Ctor branch from 8d96a6b to a504a74 Compare October 6, 2021 18:08

ChJR added 3 commits October 7, 2021 12:07

Revert "Add errno::errors on other platforms"

38426fb

This reverts commit 73795e8.

Revert "Remove cfg over crate::stdlib::errno:errors module"

179df68

This reverts commit 02946e8.

Avoid CI build error on WASI

d913c45

ChJR force-pushed the feature/OSError-Subclass-Ctor branch from 7a2eb2a to d913c45 Compare October 7, 2021 14:56

remove meaningless relocation

7f8562a

youknowone force-pushed the feature/OSError-Subclass-Ctor branch from aa3beda to 7f8562a Compare October 7, 2021 17:33

youknowone merged commit 296f183 into RustPython:main Oct 7, 2021

ChJR deleted the feature/OSError-Subclass-Ctor branch October 8, 2021 16:36

youknowone mentioned this pull request Oct 10, 2021

Fix wasi build #3265

Merged

youknowone added the z-ca-2021 Tag to track contrubution-academy 2021 label Oct 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make OSError subclass instead of itself #2960

Make OSError subclass instead of itself #2960

Uh oh!

ChJR commented Aug 24, 2021

Uh oh!

Uh oh!

youknowone Aug 26, 2021

Uh oh!

ChJR Aug 26, 2021

Uh oh!

youknowone commented Aug 26, 2021

Uh oh!

sobolevn left a comment

Uh oh!

youknowone Oct 2, 2021

Uh oh!

ChJR Oct 2, 2021

Uh oh!

youknowone Oct 4, 2021

Uh oh!

ChJR Oct 4, 2021

Uh oh!

youknowone Oct 4, 2021

Uh oh!

ChJR Oct 4, 2021

Uh oh!

youknowone Oct 4, 2021

Uh oh!

youknowone commented Oct 7, 2021

Uh oh!

Uh oh!

	macro_rules! extend_exception {
	( $ctx:expr, $class:expr, { $($name:expr => $value:expr),* $(,)* }) => {
	$class.set_str_attr("__new__", $ctx.new_method("__new__", $class.clone(), PyBaseException::tp_new));
	$class.set_str_attr("__init__", $ctx.new_method("__init__", $class.clone(), PyBaseException::init));

	extend_class!($ctx, $class, {
	$(
	$name => $value,
	)*
	});
	};
	}

Make OSError subclass instead of itself #2960

Make OSError subclass instead of itself #2960

Uh oh!

Conversation

ChJR commented Aug 24, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Aug 26, 2021

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Oct 7, 2021

Uh oh!

Uh oh!