bpo-34246: Use no mutable default args in smtplib #8554

scorphus · 2018-07-29T14:59:35Z

Some methods of the SMTP class use mutable default arguments. Specially send_message is affected in which it mutates one of the args by appending items to it, whic has side effects on further calls.

https://bugs.python.org/issue34246

serhiy-storchaka · 2018-07-29T18:24:30Z

There is nothing wrong with using mutable default arguments if they are not mutated inside function and are not exposed outside of it.

For mail_options I suggest to use an empty tuple. And instead of mail_options += ['SMTPUTF8', 'BODY=8BITMIME'] use mail_options = (*mail_options, 'SMTPUTF8', 'BODY=8BITMIME').

Some methods of the SMTP class use mutable default arguments. Specially `send_message` is affected in which it mutates one of the args by appending items to it, whic has side effects on further calls.

scorphus · 2018-08-06T08:41:44Z

Hello, @serhiy-storchaka.

Thanks so much for reviewing my changes. It's now cleaner with tuples 👍

I'm not sure whether I should ping you or if the new commit (67f70fd) triggers that automatically.

serhiy-storchaka · 2018-08-06T09:24:13Z

Please don't squash commits, this makes reviewing new changes more difficult. I have not received a notification because you edited your commit instead of adding a new commit.

serhiy-storchaka · 2018-08-06T09:26:27Z

Misc/NEWS.d/next/Library/2018-07-29-15-25-15.bpo-34246.xiKq-Q.rst

@@ -0,0 +1,2 @@
+Use an empty tuple as the default argument for mail_options and avoid it from


The argument for what function? Using an empty tuple is an implementation detail, please describe the user visible behavior change. For example ":meth:`smtplib.SMTP.send_message` no longer modifies the content of the *mail_options* argument." or something like.

Add also "Patch by Pablo S. Blum de Aguiar."

methane · 2018-08-06T09:52:38Z

Lib/smtplib.py

@@ -958,7 +958,7 @@ def send_message(self, msg, from_addr=None, to_addrs=None,
            if international:
                g = email.generator.BytesGenerator(
                    bytesmsg, policy=msg.policy.clone(utf8=True))
-                mail_options += ['SMTPUTF8', 'BODY=8BITMIME']
+                mail_options = (*mail_options, 'SMTPUTF8', 'BODY=8BITMIME')


I don't like this change. += seems more common/readable, and it's valid for immutable objects
like tuple and str. (You need to change [] to () in this case)

I'm sorry, I was wrong. When caller passed list, list += tuple will raise TypeError.

Just to ensure `smtplib.SMTP.send_message` no longer modifies the content of the *mail_options* argument

scorphus · 2018-08-16T10:05:54Z

Hey, I hope the 2 new commits I added 10 days ago triggered a notification. I also hope this message isn't just additional noise – apologies if it is, I'm still new to contrubuting to CPython.

serhiy-storchaka · 2018-08-16T10:34:06Z

Thank you for your reminder @scorphus. It is not a noise. This PR LGTM, but we need to get an approval from @bitdancer or @warsaw, the email module experts.

serhiy-storchaka

Please update also the signature in Doc/library/smtplib.rst.

bedevere-bot · 2018-08-20T08:24:51Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

scorphus · 2018-08-20T11:17:10Z

I have made the requested changes; please review again.

Regarding #5334 (and issue32657) it proposes a change that was already discussed here and droped in favour of the current status of this very PR (#8554).

bedevere-bot · 2018-08-20T11:17:12Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

serhiy-storchaka · 2018-08-20T11:25:36Z

If rcpt_options expected to be a sequence, not mapping, it is better to make a default value for options in send_message(), sendmail() and rcpt() an empty tuple than an empty list or dict.

scorphus · 2018-08-20T11:49:31Z

Yeah, I thought so – and actually did exactly that in my initial change – but decided not to, as rcpt_options is not mutated anywhere – it's just joined. Anyways, I agree it won't hurt :-)

scorphus · 2018-08-20T12:26:15Z

Done, @serhiy-storchaka

How about mail's default value for options?

serhiy-storchaka · 2018-08-20T12:38:05Z

It won't hurt :-)

Making the default value for options in mail() an empty tuple LGTM.

scorphus · 2018-08-20T13:13:33Z

Cool! Thanks, @serhiy-storchaka.

serhiy-storchaka

LGTM! Thank you for your contribution Pablo!

scorphus · 2018-08-21T08:25:47Z

YAY! Thanks for reviewing! I'm very happy to contribute!

vstinner

LGTM.

miss-islington · 2018-09-07T22:04:51Z

Thanks @scorphus for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

Some methods of the SMTP class use mutable default arguments. Specially `send_message` is affected as it mutates one of the args by appending items to it, which has side effects on further calls. (cherry picked from commit d5fbe9b) Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

bedevere-bot · 2018-09-07T22:04:58Z

GH-9111 is a backport of this pull request to the 3.7 branch.

miss-islington · 2018-09-07T22:04:59Z

Sorry, @scorphus and @pablogsal, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker d5fbe9b1a3d65ceeb9159c5ba999ee966a945f76 3.6

Some methods of the SMTP class use mutable default arguments. Specially `send_message` is affected as it mutates one of the args by appending items to it, which has side effects on further calls. (cherry picked from commit d5fbe9b) Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

bedevere-bot · 2018-09-07T23:12:17Z

GH-9112 is a backport of this pull request to the 3.6 branch.

Some methods of the SMTP class use mutable default arguments. Specially `send_message` is affected as it mutates one of the args by appending items to it, which has side effects on further calls.. (cherry picked from commit d5fbe9b) Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

) Some methods of the SMTP class use mutable default arguments. Specially `send_message` is affected as it mutates one of the args by appending items to it, which has side effects on further calls.. (cherry picked from commit d5fbe9b) Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

scorphus requested a review from a team as a code owner July 29, 2018 14:59

the-knights-who-say-ni added the CLA signed label Jul 29, 2018

bedevere-bot added the awaiting review label Jul 29, 2018

scorphus force-pushed the fix-issue-34246 branch from 49247c0 to 2975697 Compare August 2, 2018 12:53

bpo-34246: Use tuple as default arg for mail_options

67f70fd

Some methods of the SMTP class use mutable default arguments. Specially `send_message` is affected in which it mutates one of the args by appending items to it, whic has side effects on further calls.

scorphus force-pushed the fix-issue-34246 branch from 2975697 to 67f70fd Compare August 2, 2018 12:56

serhiy-storchaka reviewed Aug 6, 2018

View reviewed changes

methane reviewed Aug 6, 2018

View reviewed changes

scorphus added 2 commits August 6, 2018 13:41

Change NEWS file content as suggested

e2e4e5f

Add one more test case

72b15ce

Just to ensure `smtplib.SMTP.send_message` no longer modifies the content of the *mail_options* argument

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Aug 16, 2018

serhiy-storchaka requested changes Aug 20, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Aug 20, 2018

scorphus added 2 commits August 20, 2018 13:09

Make rcpt_options compatible with what sendmail expects

ca6176e

Update method signature in Doc/library/smtplib.rst

cdf9fd3

bedevere-bot removed the awaiting changes label Aug 20, 2018

bedevere-bot added the awaiting change review label Aug 20, 2018

Make empty tuple as values for options arguments

0324dbb

Use empty tuple as default value for options in mail

1d4621f

serhiy-storchaka approved these changes Aug 20, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 20, 2018

vstinner approved these changes Sep 7, 2018

View reviewed changes

pablogsal merged commit d5fbe9b into python:master Sep 7, 2018

bedevere-bot removed the awaiting merge label Sep 7, 2018

bedevere-bot removed the needs backport to 3.7 label Sep 7, 2018

miss-islington assigned pablogsal Sep 7, 2018

bedevere-bot removed the needs backport to 3.6 label Sep 7, 2018

methane mentioned this pull request Feb 22, 2019

bpo-32657 Removing mutable entry from send_message #5334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-34246: Use no mutable default args in smtplib #8554

bpo-34246: Use no mutable default args in smtplib #8554

scorphus commented Jul 29, 2018 •

edited by bedevere-bot

Loading

serhiy-storchaka commented Jul 29, 2018

scorphus commented Aug 6, 2018

serhiy-storchaka commented Aug 6, 2018

serhiy-storchaka Aug 6, 2018

methane Aug 6, 2018 •

edited

Loading

scorphus commented Aug 16, 2018

serhiy-storchaka commented Aug 16, 2018

serhiy-storchaka left a comment

bedevere-bot commented Aug 20, 2018

scorphus commented Aug 20, 2018

bedevere-bot commented Aug 20, 2018

serhiy-storchaka commented Aug 20, 2018

scorphus commented Aug 20, 2018

scorphus commented Aug 20, 2018

serhiy-storchaka commented Aug 20, 2018 •

edited

Loading

scorphus commented Aug 20, 2018

serhiy-storchaka left a comment

scorphus commented Aug 21, 2018

vstinner left a comment

miss-islington commented Sep 7, 2018

bedevere-bot commented Sep 7, 2018

miss-islington commented Sep 7, 2018

bedevere-bot commented Sep 7, 2018

		@@ -0,0 +1,2 @@
		Use an empty tuple as the default argument for mail_options and avoid it from

bpo-34246: Use no mutable default args in smtplib #8554

bpo-34246: Use no mutable default args in smtplib #8554

Conversation

scorphus commented Jul 29, 2018 • edited by bedevere-bot Loading

serhiy-storchaka commented Jul 29, 2018

scorphus commented Aug 6, 2018

serhiy-storchaka commented Aug 6, 2018

serhiy-storchaka Aug 6, 2018

Choose a reason for hiding this comment

methane Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

scorphus commented Aug 16, 2018

serhiy-storchaka commented Aug 16, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 20, 2018

scorphus commented Aug 20, 2018

bedevere-bot commented Aug 20, 2018

serhiy-storchaka commented Aug 20, 2018

scorphus commented Aug 20, 2018

scorphus commented Aug 20, 2018

serhiy-storchaka commented Aug 20, 2018 • edited Loading

scorphus commented Aug 20, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

scorphus commented Aug 21, 2018

vstinner left a comment

Choose a reason for hiding this comment

miss-islington commented Sep 7, 2018

bedevere-bot commented Sep 7, 2018

miss-islington commented Sep 7, 2018

bedevere-bot commented Sep 7, 2018

scorphus commented Jul 29, 2018 •

edited by bedevere-bot

Loading

methane Aug 6, 2018 •

edited

Loading

serhiy-storchaka commented Aug 20, 2018 •

edited

Loading