fix: "invalid settings.json" message #4054

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

@@ -16,6 +16,9 @@ import { ToolkitError } from './errors'
 
 type Workspace = Pick<typeof vscode.workspace, 'getConfiguration' | 'onDidChangeConfiguration'>
 
+/** Used by isValid(). Must be something that's defined in our package.json. */
+const testSetting = 'aws.samcli.lambdaTimeout'
+
 /**
  * A class for manipulating VS Code user settings (from all extensions).
  *
@@ -86,39 +89,46 @@ 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> {
-        const key = 'aws.samcli.lambdaTimeout'
+    public async isValid(): Promise<'ok' | 'invalid' | 'nowrite'> {
+        const key = testSetting
         const config = this.getConfig()
+        const tempValOld = 1234 // Legacy temp value we are migrating from.
+        const tempVal = 91234 // Temp value used to check that read/write works.
         const defaultVal = settingsProps[key].default
 
         try {
-            const oldVal = config.get<number>(key)
-            // Try to write to settings.json.
-            await config.update(key, 1234, this.updateTarget)
-            // Restore the old value, if any.
-            if (oldVal === undefined || defaultVal === oldVal) {
-                // vscode will return the default even if there was no entry in settings.json.
-                // Avoid polluting the user's settings.json unnecessarily.
+            const userVal = config.get<number>(key)
+            // Try to write a temporary "sentinel" value to settings.json.
+            await config.update(key, tempVal, this.updateTarget)
+            if (userVal === undefined || [defaultVal, tempValOld, tempVal].includes(userVal)) {
+                // Avoid polluting the user's settings.json.
                 await config.update(key, undefined, this.updateTarget)
             } else {
-                await config.update(key, oldVal, this.updateTarget)
+                // Restore the user's actual setting value.
+                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'
         }
     }
 
@@ -379,7 +389,9 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
                 }
 
                 for (const key of props.filter(isDifferent)) {
-                    this.log('key "%s" changed', key)
+                    if (`${section}.${key}` !== testSetting) {
+                        this.log('key "%s" changed', key)
+                    }
                     emitter.fire({ key })
                 }
             })

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> {