More fixes to cpp_locals

[da-woods]

1. Fix getting an attribute from a function where cpp_locals==False from a class where cpp_locals==True.
2. Added a cpp_locals tag to the test-suite that causes a file to get rerun with the cpp_locals directive on, and set it in suitable files. This should improve the test coverage for the feature. (Most of the files where it isn't set just have "local variable referenced before assignment" errors, which is expected and correct). There are a couple of real bugs that it's exposed though.
3. Some of the issues in [BUG] remaining bugs with cpp_locals directive #4266 (iteration, closures, fake reference)

[scoder]

2. Added a `cpp_locals` tag to the test-suite that causes a file to get rerun with the cpp_locals directive on, and set it in suitable files. This should improve the test coverage for the feature. (Most of the files where it isn't set just have "local variable referenced before assignment" errors, which is expected and correct). There are a couple of real bugs that it's exposed though.

So, we can't just do that test duplication automatically for all C++ specific tests?

[scoder]

2. Added a cpp_locals tag to the test-suite that causes a file to get rerun with the cpp_locals directive on, and set it in suitable files. This should improve the test coverage for the feature. (Most of the files where it isn't set just have "local variable referenced before assignment" errors, which is expected and correct). There are a couple of real bugs that it's exposed though.

So, we can't just do that test duplication automatically for all C++ specific tests?

Or, if you want higher test coverage, have a tag no_cpp_locals that disables that duplication if necessary?

[da-woods]

2. Added a `cpp_locals` tag to the test-suite that causes a file to get rerun with the cpp_locals directive on, and set it in suitable files. This should improve the test coverage for the feature. (Most of the files where it isn't set just have "local variable referenced before assignment" errors, which is expected and correct). There are a couple of real bugs that it's exposed though.

So, we can't just do test duplication automatically that for all C++ specific tests?

The trouble is that the very common:

cdef CppClass c
c.attr()

doesn't work because c is uninitialized (which is correct - it's now behaving like a Python object). Most of the time it'd be possible to change the test, but there's quite a few of them and some may be deliberately testing the old behaviour.

Or, if you want higher test coverage, have a tag no_cpp_locals that disables that duplication if necessary?

Yeah - opt-out could possibly be better than opt-in. It at least ensures that new tests are written to work in both (unless there's good reason)

[scoder]

1. Shouldn't analyse_rvalue_entry() do this?
2. Do we really (really!) need a new and completely option specific field in NameNode (and AttributeNode, as it seems) for this? Or is it information that we can infer from some existing state? (It's an rvalue, it's cpp-optional, so …)

[da-woods]

Or is it information that we can infer from some existing state? (It's an rvalue, it's cpp-optional, so …)

In principle the .is_target attribute should tell us that it's a rvalue. In practice that doesn't look to be set (ever). I've changed it so that .is_target is actually set and intered it as you suggest.

[scoder]

Can this use the with gil function declaration? (And I also wonder how this ever worked…)

[da-woods]

And I also wonder how this ever worked…

I have a vague feeling that some simple print statements are able to work without the GIL (by design?). But it for some reason it stopped working, and it definitely isn't the point of the test so I didn't investigate further

[scoder]

I have a vague feeling that some simple print statements are able to work without the GIL (by design?)

Ah, sure. You switched the test from Py2 to Py3, which turned the print statement into a print() function call. Print statements implicitly acquire the GIL, for convenience. Print function calls are not so easy to spot. See the InjectGILHandling transform.

[da-woods]

I've made the tests opt-in. I'm expecting to be caught out by a few in the compile directory because I haven't looked at any of those yet

[scoder]

Hmm. I don't know how to do this better right now, but it's probably not something that you're proud of either.

[da-woods]

No this isn't great.

Another option might be to add an optional argument to AttributeNote.result to get the result with/without dereferencing. Breaks the general interface though, but my making it an optional argument other callers shouldn't notice. I'm not sure which is better.

[scoder]

To me, it feels more like calling .result() isn't the right thing and there should be some intermediate method. The result() method is clearly doing too much here. Adding an option to it would rather make it worse.

I could imagine splitting most of calculate_result_code() out into a new method calculate_access_code(), and then calling that from calculate_result_code() and from here.

[scoder]

Hmm. A 'language' doesn't really seem the right way to express this. We already split tests for .py files. Are the requirements for this tag really different?

[da-woods]

I agree it's not quite a language. The nice thing about misusing language is that it creates them in a separate cpp_locals directory and reports the tests as cpp_locals/testnames without too much extra effort.

The approach we use for the .py files doesn't quite work - that creates a separate PureDoctestTestCase at a much earlier stage before the CythonRunTestCase is created (which makes sense there because it's quite a different test, but doesn't make so much sense here where it's largely the same test but with different options).

What would work is changing:

tests = [ self.build_test(test_class, path, workdir, module, module_path,
                                  tags, language, language_level,
                                  expect_errors, expect_warnings, warning_errors, preparse,
                                  pythran_dir if language == "cpp" else None,
                                  add_cython_import=add_cython_import)
                  for language in languages
                  for preparse in preparse_list
                  for language_level in language_levels
        ]

to

tests = [ self.build_test(test_class, path, workdir, module, module_path,
                                  tags, language, language_level,
                                  expect_errors, expect_warnings, warning_errors, preparse,
                                  pythran_dir if language == "cpp" else None,
                                  add_cython_import=add_cython_import,
                                  cpp_locals_on=cpp_locals_on)
                  for language in languages
                  for preparse in preparse_list
                  for language_level in language_levels
                  for cpp_locals_on in cpp_locals_states
        ]

(plus a few supporting changes elsewhere). To me that feels more intrusive that using 'language', but I'm happy to make that change if you disagree.

[scoder]

This is pretty much what needs to be done, I think, but we should use a list of extra_directives dicts. Given that we will eventually need to test at least some directives separately or in combination, I think this one isn't special enough to merit its own option.

You can keep the no_cpp_locals tag by simply checking if the part without the no_ is in the extra directives.

[da-woods]

Yeah a generic solution is probably better so I've done that. In principle it might be possible to handle language level in the extra directives too but I decided it was safer not to touch that for now.

[scoder]

To me, it feels more like calling .result() isn't the right thing and there should be some intermediate method. The result() method is clearly doing too much here. Adding an option to it would rather make it worse.

I could imagine splitting most of calculate_result_code() out into a new method calculate_access_code(), and then calling that from calculate_result_code() and from here.

[scoder]

I'm not sure if this comment adds anything that's not obvious from the line that follows. An explanation of the reason would help more.

Suggested change

	# exclude fake references from this because they aren't replaced by optional
	# Fake references are not replaced with "std::optional()".