Repository: cython/cython
Pull request: Treat annotations in functions in dataclasses normally
Fixes #4953.

Before this change the annotations were being treated as
"dataclass-related" annotations, and thus being made "public".

Problem statement:
[BUG] Can't provide type for local variable in method of Dataclass
```cython
@cython.dataclasses.dataclass
@cython.cclass
class MyClass:
    x : cython.int

    def x_times_2(self):
        x2 : cython.int
        x2 = self.x * 2
        return x2
```

This fails to compile (on 64-bit linux with Python 3.8.11 and Cython 3.0.0 Alpha 11) with the following error for the line that specifies the type of x2: `Local variable cannot be declared public`

If I remove that line or remove the dataclass decorator from the class, it compiles.

Hints from issue discussion:

Commit message: Remove comment that relates to changes I didn't make

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index bfe065171bb..493fd140c54 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -2057,15 +2057,10 @@ def declare_from_annotation(self, env, as_target=False):
                     atype = error_type
 
             visibility = 'private'
-            if 'dataclasses.dataclass' in env.directives:
+            if env.is_c_class_scope and env.is_dataclass:
                 # handle "frozen" directive - full inspection of the dataclass directives happens
                 # in Dataclass.py
-                frozen_directive = None
-                dataclass_directive = env.directives['dataclasses.dataclass']
-                if dataclass_directive:
-                    dataclass_directive_kwds = dataclass_directive[1]
-                    frozen_directive = dataclass_directive_kwds.get('frozen', None)
-                is_frozen = frozen_directive and frozen_directive.is_literal and frozen_directive.value
+                is_frozen = env.is_dataclass == "frozen"
                 if atype.is_pyobject or atype.can_coerce_to_pyobject(env):
                     visibility = 'readonly' if is_frozen else 'public'
                     # If the object can't be coerced that's fine - we just don't create a property
@@ -2160,7 +2155,7 @@ def _analyse_target_declaration(self, env, is_assignment_expression):
             self.entry.known_standard_library_import = ""  # already exists somewhere and so is now ambiguous
         if not self.entry and self.annotation is not None:
             # name : type = ...
-            is_dataclass = 'dataclasses.dataclass' in env.directives
+            is_dataclass = env.is_c_class_scope and env.is_dataclass
             # In a dataclass, an assignment should not prevent a name from becoming an instance attribute.
             # Hence, "as_target = not is_dataclass".
             self.declare_from_annotation(env, as_target=not is_dataclass)
diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index 50436561bde..cc719fba045 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -5169,7 +5169,6 @@ class CClassDefNode(ClassDefNode):
     check_size = None
     decorators = None
     shadow = False
-    is_dataclass = False
 
     @property
     def punycode_class_name(self):
@@ -5219,8 +5218,6 @@ def analyse_declarations(self, env):
 
         if env.in_cinclude and not self.objstruct_name:
             error(self.pos, "Object struct name specification required for C class defined in 'extern from' block")
-        if "dataclasses.dataclass" in env.directives:
-            self.is_dataclass = True
         if self.decorators:
             error(self.pos, "Decorators not allowed on cdef classes (used on type '%s')" % self.class_name)
         self.base_type = None
@@ -5310,6 +5307,16 @@ def analyse_declarations(self, env):
         self.scope = scope = self.entry.type.scope
         if scope is not None:
             scope.directives = env.directives
+            if "dataclasses.dataclass" in env.directives:
+                is_frozen = False
+                dataclass_directives = env.directives["dataclasses.dataclass"]
+                if dataclass_directives:
+                    frozen_directive = dataclass_directives[1].get('frozen', None)
+                    is_frozen = frozen_directive and frozen_directive.is_literal and frozen_directive.value
+                if is_frozen:
+                    scope.is_dataclass = "frozen"
+                else:
+                    scope.is_dataclass = True
 
         if self.doc and Options.docstrings:
             scope.doc = embed_position(self.pos, self.doc)
diff --git a/Cython/Compiler/Options.py b/Cython/Compiler/Options.py
index bb547e97809..73778aaf90e 100644
--- a/Cython/Compiler/Options.py
+++ b/Cython/Compiler/Options.py
@@ -391,7 +391,7 @@ class DEFER_ANALYSIS_OF_ARGUMENTS:
 # a list of directives that (when used as a decorator) are only applied to
 # the object they decorate and not to its children.
 immediate_decorator_directives = {
-    'cfunc', 'ccall', 'cclass',
+    'cfunc', 'ccall', 'cclass', 'dataclasses.dataclass',
     # function signature directives
     'inline', 'exceptval', 'returns',
     # class directives
diff --git a/Cython/Compiler/ParseTreeTransforms.py b/Cython/Compiler/ParseTreeTransforms.py
index ae271094ae6..f17af31f180 100644
--- a/Cython/Compiler/ParseTreeTransforms.py
+++ b/Cython/Compiler/ParseTreeTransforms.py
@@ -1318,8 +1318,7 @@ def _extract_directives(self, node, scope_name):
                         name, value = directive
                         if self.directives.get(name, object()) != value:
                             directives.append(directive)
-                        if (directive[0] == 'staticmethod' or
-                                (directive[0] == 'dataclasses.dataclass' and scope_name == 'class')):
+                        if directive[0] == 'staticmethod':
                             both.append(dec)
                     # Adapt scope type based on decorators that change it.
                     if directive[0] == 'cclass' and scope_name == 'class':
diff --git a/Cython/Compiler/Symtab.py b/Cython/Compiler/Symtab.py
index 1500c7441a0..0b44959b9aa 100644
--- a/Cython/Compiler/Symtab.py
+++ b/Cython/Compiler/Symtab.py
@@ -2258,6 +2258,7 @@ class CClassScope(ClassScope):
     #  defined               boolean  Defined in .pxd file
     #  implemented           boolean  Defined in .pyx file
     #  inherited_var_entries [Entry]  Adapted var entries from base class
+    #  is_dataclass     boolean or "frozen"  is a cython.dataclasses.dataclass
 
     is_c_class_scope = 1
     is_closure_class_scope = False
@@ -2268,6 +2269,7 @@ class CClassScope(ClassScope):
     has_cyclic_pyobject_attrs = False
     defined = False
     implemented = False
+    is_dataclass = False
 
     def __init__(self, name, outer_scope, visibility):
         ClassScope.__init__(self, name, outer_scope)
@@ -2345,7 +2347,7 @@ def declare_var(self, name, type, pos,
                         type = py_object_type
                     else:
                         type = type.equivalent_type
-            if  "dataclasses.InitVar" in pytyping_modifiers and 'dataclasses.dataclass' not in self.directives:
+            if  "dataclasses.InitVar" in pytyping_modifiers and not self.is_dataclass:
                 error(pos, "Use of cython.dataclasses.InitVar does not make sense outside a dataclass")
 
         if is_cdef:
diff --git a/tests/run/pure_cdef_class_dataclass.py b/tests/run/pure_cdef_class_dataclass.py
index b3892586fc7..7ba390d3977 100644
--- a/tests/run/pure_cdef_class_dataclass.py
+++ b/tests/run/pure_cdef_class_dataclass.py
@@ -25,11 +25,18 @@ class MyDataclass:
     True
     >>> hash(inst1) != id(inst1)
     True
+    >>> inst1.func_with_annotations(2.0)
+    4.0
     """
 
     a: int = 1
     self: list = cython.dataclasses.field(default_factory=list, hash=False)  # test that arguments of init don't conflict
 
+    def func_with_annotations(self, b: float):
+        c: float = b
+        return self.a * c
+
+
 
 class DummyObj:
     def __repr__(self):


Code review comments:
diff_hunk: @@ -2160,7 +2155,7 @@ def _analyse_target_declaration(self, env, is_assignment_expression):
             self.entry.known_standard_library_import = ""  # already exists somewhere and so is now ambiguous
         if not self.entry and self.annotation is not None:
             # name : type = ...
-            is_dataclass = 'dataclasses.dataclass' in env.directives
+            is_dataclass = env.is_c_class_scope and env.is_dataclass
path: Cython/Compiler/ExprNodes.py
text: @user1:
Do we need both conditions? Doesn't `is_dataclass` always imply `is_c_class_scope`? Or are you anticipating some kind of support for Python dataclasses as well?

@author:
It's more that py class scopes won't have the attribute

@user1:
Ah, ok, that makes sense.

However, we already have a special `is_closure_class_scope`, so maybe we could call this one `is_c_dataclass_scope` and promote both to `Scope` class attributes? Maybe they are just special enough to make them independent of `is_c_class_scope`.

diff_hunk: @@ -5310,6 +5307,16 @@ def analyse_declarations(self, env):
         self.scope = scope = self.entry.type.scope
         if scope is not None:
             scope.directives = env.directives
+            if "dataclasses.dataclass" in env.directives:
+                is_frozen = False
+                dataclass_directives = env.directives["dataclasses.dataclass"]
+                if dataclass_directives:
+                    frozen_directive = dataclass_directives[1].get('frozen', None)
path: Cython/Compiler/Nodes.py
text: @user1:
Hmm, whatever `dataclass_directives[1]` is – probably some kind of dict, but reading code like this suggests that there might be a more descriptive way to represent these "directives".

Note that you don't need to spell out the `None` fallback value for `dict.get()`.
```suggestion
                    frozen_directive = dataclass_directives[1].get('frozen')
```

@user1:
(I was referring to the `dataclass_directives` … tuple, maybe? Saying `something[1]` simply doesn't give much information about what we're getting as a result. And I don't mean so much the type of the result as the semantic meaning. Attributes are much clearer since they give a name to the thing that we're looking at. That's why named tuples are better than regular tuples in some cases. Well, and dataclasses.)

diff_hunk: @@ -5310,6 +5307,16 @@ def analyse_declarations(self, env):
         self.scope = scope = self.entry.type.scope
         if scope is not None:
             scope.directives = env.directives
+            if "dataclasses.dataclass" in env.directives:
+                is_frozen = False
+                dataclass_directives = env.directives["dataclasses.dataclass"]
+                if dataclass_directives:
+                    frozen_directive = dataclass_directives[1].get('frozen', None)
+                    is_frozen = frozen_directive and frozen_directive.is_literal and frozen_directive.value
+                if is_frozen:
+                    scope.is_dataclass = "frozen"
+                else:
+                    scope.is_dataclass = True
path: Cython/Compiler/Nodes.py
text: @user1:
I really like conditional expressions for cases like this because they underline the intention to set a variable or attribute, just in different ways depending on a condition.
```suggestion
                scope.is_dataclass = "frozen" if is_frozen else True
```

diff_hunk: @@ -5310,6 +5307,16 @@ def analyse_declarations(self, env):
         self.scope = scope = self.entry.type.scope
         if scope is not None:
             scope.directives = env.directives
+            if "dataclasses.dataclass" in env.directives:
+                is_frozen = False
+                dataclass_directives = env.directives["dataclasses.dataclass"]
+                if dataclass_directives:
+                    frozen_directive = dataclass_directives[1].get('frozen', None)
+                    is_frozen = frozen_directive and frozen_directive.is_literal and frozen_directive.value
path: Cython/Compiler/Nodes.py
text: @user1:
We probably already know the node's `constant_result` at this point. (You can ask `has_constant_result()`, but there's hopefully some prior piece of validation code that makes sure that we *do* know the constant result of this flag at compile time.)

@author:
I've changed to using `constant_result`. We do currently enforce `is_literal` on these (https://github.com/cython/cython/blob/5ddbe05d419d48161e5938e8de227d41913312ee/Cython/Compiler/Dataclass.py#L181) but I guess `constant_result` is one less thing to change if we change that rule later

@author:
Don't think `constant_result` will work. I think directives are taken out of the visited tree before the `ConstantFolding` step so `is_literal` is probably the only way to do it.