feat(settings): "invalid settings.json" message for readonly settings

Problem:
settings.json may be intentionally readonly, then "invalid
settings.json" is not helpful. #4043

Solution:
Only log a warning if settings.json is not writable, don't show
a message. If settings.json is invalid a message is still shown.

Author: justinmk3

.changes/next-release/Feature-9f098b03-c8ed-4137-80cb-e7508d42a97f.json

@@ -0,0 +1,4 @@
+{
+	"type": "Feature",
+	"description": "Don't show \"invalid settings.json\" message if settings.json is valid but unwritable"
+}

src/extension.ts

@@ -408,17 +408,28 @@ export function logAndShowWebviewError(err: unknown, webviewId: string, command:
 }
 
 async function checkSettingsHealth(settings: Settings): Promise<boolean> {
-    const ok = await settings.isValid()
-    if (!ok) {
-        const msg = 'User settings.json file appears to be invalid. Check settings.json for syntax errors.'
-        const openSettingsItem = 'Open settings.json'
-        showViewLogsMessage(msg, 'error', [openSettingsItem]).then(async resp => {
-            if (resp === openSettingsItem) {
-                vscode.commands.executeCommand('workbench.action.openSettingsJson')
-            }
-        })
+    const r = await settings.isValid()
+    switch (r) {
+        case 'invalid': {
+            const msg = 'Failed to access settings. Check settings.json for syntax errors.'
+            const openSettingsItem = 'Open settings.json'
+            showViewLogsMessage(msg, 'error', [openSettingsItem]).then(async resp => {
+                if (resp === openSettingsItem) {
+                    vscode.commands.executeCommand('workbench.action.openSettingsJson')
+                }
+            })
+            return false
+        }
+        // Don't show a message for 'nowrite' because:
+        //  - settings.json may intentionally be readonly. #4043
+        //  - vscode will show its own error if settings.json cannot be written.
+        //
+        // Note: isValid() already logged a warning.
+        case 'nowrite':
+        case 'ok':
+        default:
+            return true
     }
-    return ok
 }
 
 async function getMachineId(): Promise<string> {

src/shared/settings.ts

@@ -86,13 +86,13 @@ export class Settings {
     }
 
     /**
-     * Checks that the user `settings.json` works correctly. #3910
+     * Checks that user `settings.json` actually works. #3910
      *
-     * Note: This checks that we can actually _write_ settings. vscode notifies the user if
-     * settings.json is complete nonsense, but is silent if there are only "recoverable" JSON syntax
-     * errors.
+     * Note: This checks that we can actually "roundtrip" (read and write) settings. vscode notifies
+     * the user if settings.json is complete nonsense, but silently fails if there are only
+     * "recoverable" JSON syntax errors.
      */
-    public async isValid(): Promise<boolean> {
+    public async isValid(): Promise<'ok' | 'invalid' | 'nowrite'> {
         const key = 'aws.samcli.lambdaTimeout'
         const config = this.getConfig()
         const tempValOld = 1234 // Legacy temp value we are migrating from.
@@ -111,15 +111,21 @@ export class Settings {
                 await config.update(key, userVal, this.updateTarget)
             }
 
-            return true
+            return 'ok'
         } catch (e) {
-            getLogger().warn(
-                'settings: invalid "settings.json" file? failed to update "%s": %s',
-                key,
-                (e as Error).message
-            )
+            const err = e as Error
+            // If anything tries to update an unwritable settings.json, vscode will thereafter treat
+            // it as an "unsaved" file. #4043
+            if (err.message.includes('EACCES') || err.message.includes('the file has unsaved changes')) {
+                const logMsg = 'settings: unwritable settings.json: %s'
+                getLogger().warn(logMsg, err.message)
+                return 'nowrite'
+            }
 
-            return false
+            const logMsg = 'settings: invalid "settings.json": %s'
+            getLogger().error(logMsg, err.message)
+
+            return 'invalid'
         }
     }
 

src/test/shared/settingsConfiguration.test.ts

@@ -4,6 +4,7 @@
  */
 
 import assert from 'assert'
+import * as sinon from 'sinon'
 import * as vscode from 'vscode'
 import { DevSettings, Experiments, fromExtensionManifest, PromptSettings, Settings } from '../../shared/settings'
 import { TestSettings } from '../utilities/testSettingsConfiguration'
@@ -26,6 +27,10 @@ describe('Settings', function () {
         await sut.update(settingKey, undefined)
     })
 
+    afterEach(async function () {
+        sinon.restore()
+    })
+
     const scenarios = [
         { testValue: 1234, desc: 'number' },
         { testValue: 0, desc: 'default number' },
@@ -41,6 +46,31 @@ describe('Settings', function () {
         // Note: we don't test undefined because retrieval returns the package.json configured default value, if there is one
     ]
 
+    it('isValid()', async () => {
+        // TODO: could avoid sinon if we can force vscode to use a dummy settings.json file.
+        const fake = {
+            get: async () => {
+                return 'setting-value'
+            },
+            update: async () => {
+                throw Error()
+            },
+        } as unknown as vscode.WorkspaceConfiguration
+        sinon.stub(vscode.workspace, 'getConfiguration').returns(fake)
+
+        assert.deepStrictEqual(await sut.isValid(), 'invalid')
+
+        fake.update = async () => {
+            throw Error('EACCES')
+        }
+        assert.deepStrictEqual(await sut.isValid(), 'nowrite')
+
+        fake.update = async () => {
+            throw Error('xxx the file has unsaved changes xxx')
+        }
+        assert.deepStrictEqual(await sut.isValid(), 'nowrite')
+    })
+
     describe('get', function () {
         let settings: vscode.WorkspaceConfiguration
 

src/test/utilities/testSettingsConfiguration.ts

@@ -48,8 +48,8 @@ export class TestSettings implements ClassToInterfaceType<Settings> {
         return !type || value === undefined ? value : cast(value, type)
     }
 
-    public async isValid(): Promise<boolean> {
-        return true
+    public async isValid(): Promise<'ok' | 'invalid' | 'nowrite'> {
+        return 'ok'
     }
 
     public async update(key: string, value: unknown): Promise<boolean> {