timezone.utc isn't replaced when datetime isn't imported

[browniebroke]

Python Version

3.11

Django Version

4.2

Package Version

1.15.0

Description

I have a case where timezone.utc isn't replaced by django-upgrade. This was introduced in #169 (requested in #135) and as far as I can tell, my use case isn't part of the tests, but I'm not sure how feasible it is:

from django.utils import timezone
myfunction(date_created, tz=timezone.utc)

I assume it's because the value is a keyword argument? I would expect it to replace it as:

+ import datetime
+ myfunction(date_created, tz=datetime.timezone.utc)
- from django.utils import timezone
- myfunction(date_created, tz=timezone.utc)

Although now that I look more closely, my use case is slightly more complex, as the file also uses timezone.now(), in case it matters. All in all, the expected behaviour would probably be:

+ import datetime
  from django.utils import timezone

  now = timezone.now()
- myfunction(date_created, tz=timezone.utc)
+ myfunction(date_created, tz=datetime.timezone.utc)

[browniebroke]

Actually I found a test case which matches my use case:

django-upgrade/tests/fixers/test_utils_timezone.py

Lines 45 to 53 in 5afcc45

	def test_no_datetime_import():
	check_noop(
	"""\
	from django.utils import timezone
	do_a_thing(timezone.utc)
	""",
	settings,
	)

I have timezone imported, but no datetime import. I can see that test case was added in #227. Looks like it was intentionally decided to not handle this case, I assume there are some complications with adding a datetime import...

[UnknownPlatypus]

See #389 (comment) and related issue for a similar problem, adding import is non trivial because it could be shadowed by anything named datetime in scope. safely doing that imply tracking scopes and declared named I suppose

[browniebroke]

Makes sense. I see that it's actually listed as a requirement for this fixer to work in the README. Going to close this, as I don't have a good suggestion on how to implement this safely... It seems relatively unlikely that something in scope is called datetime without having datetime imported, but it's probably better to err on the safe side.

As an inspiration, we might look at libCST, which has a fixer to add import.

[UnknownPlatypus]

Their fixer seems to have the same issue, they don't really check scopes because it would be too expensive. See Instagram/LibCST#1024 (comment)

I agree, AddImportsVisitor won't be able to do a perfect job all the time unless we include expensive scope analysis into it