Add namereplace error handler #1140
Conversation
|
Today I am AFK, but can look at it tomorrow. |
| @@ -2638,6 +2644,69 @@ private class ExceptionFallbackBufferUtf8DotNet : ExceptionFallbackBuffer { | |||
| } | |||
| } | |||
|
|
|||
| private static object NameReplaceErrors(object unicodeError) { | |||
| Modules.unicodedata.PerformModuleReload(null, null); | |||
There was a problem hiding this comment.
There is a similar call in LiteralParser, which has been bothering me already for some time, since I had thought of enabling nullable annotations in unicodedata.cs. The old-style nullable annotations in the definition of unicodedata.PerformModuleReload mark both parameters as not-null. And indeed, PythonContext seems to be expected to be not null everywhere in the code base. It appears that in both and only cases of the explicit call to PythonModuleReload the goal is simply to make sure the private static variable ucd_5_2_0 is properly initialized. So perhaps factor that out of PythonModuleReload into a parameterless internal method? I don't know what the [SpecialName] PerformModuleReload is supposed to represent in the general case to decide whether it is OK for the parameters to become nullable or not.
There was a problem hiding this comment.
I'm fine with making an internal method to initialize ucd_5_2_0. Presumably the analyzer should handle [SpecialName] PerformModuleReload in a PythonModule and check that the arguments are non-null (and don't have [NotNull] since it's not Python callable).
While on the subject of NotNull I've also been thinking about the conflict with the standard nullable annotation attribute. I wonder if we should create a new attribute to avoid the conflict. Unfortunately I can't come up with a good name for this new attribute. Perhaps DlrNotNull, although it's likely to prevent any future collisions it's not super descriptive.
Something to keep in mind for the future, if that if we want to mimic CPython, whenever we use the namereplace error handler or \N in a string we should load the unicodedata module in sys.modules. So something more along the lines of what we do with PythonContext.EnsureEncodings (although it's slightly different since unicodedata is embedded in our main library instead of being an external module).
There was a problem hiding this comment.
While on the subject of
NotNullI've also been thinking about the conflict with the standard nullable annotation attribute. I wonder if we should create a new attribute to avoid the conflict. Unfortunately I can't come up with a good name for this new attribute. PerhapsDlrNotNull, although it's likely to prevent any future collisions it's not super descriptive.
I was wondering the same. Some time ago I have made a suggestion naming it NotNone but it is Python-centric... DLR itself calls a null type DynamicNull, but [NotDynamicNull] looks too long... NotDNull?
There was a problem hiding this comment.
NotDNull seems a bit obscure. I think I could live with NotDynamicNull...
There was a problem hiding this comment.
I think I could live with that too...
|
Looks good to me, except for a small comment. |
Resolves #1114